From d815965ebe77a35abbc02e72e529503d86f9e347 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 15:25:21 -0400
Subject: [PATCH 01/11] 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.
---
Darling/Darling.Tests/AlertEngineTests.cs | 114 +++-
.../AlertReadFailureSurfaceTests.cs | 613 ++++++++++++++++++
.../DarlingSelfAlertEvaluator.cs | 33 +-
.../DarlingWorker.cs | 37 +-
.../Mcp/DarlingMcpDataTools.cs | 43 +-
.../wwwroot/js/pages/server-tabs.js | 37 ++
Lite.Tests/AlertReadFailureSurfaceTests.cs | 275 ++++++++
Lite/MainWindow.xaml.cs | 10 +-
Lite/Mcp/McpHealthTools.cs | 45 +-
PerformanceMonitor.Alerting/AlertEngine.cs | 42 +-
.../AlertReadFailureCounter.cs | 298 +++++++++
11 files changed, 1539 insertions(+), 8 deletions(-)
create mode 100644 Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
create mode 100644 Lite.Tests/AlertReadFailureSurfaceTests.cs
create mode 100644 PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
diff --git a/Darling/Darling.Tests/AlertEngineTests.cs b/Darling/Darling.Tests/AlertEngineTests.cs
index 65ef5f304..a9cfc0b16 100644
--- a/Darling/Darling.Tests/AlertEngineTests.cs
+++ b/Darling/Darling.Tests/AlertEngineTests.cs
@@ -290,6 +290,11 @@ private sealed class Harness
public bool Muted { get; set; }
public DateTime Now { get; set; } = new(2026, 7, 1, 12, 0, 0, DateTimeKind.Utc);
+ /* #3013: the swallowed-read counter is an OPTIONAL harness input, defaulting to null, so every
+ pin above builds an engine that counts nothing and no test can leak into another's totals or
+ into the process-wide AlertReadFailureCounter.Shared. */
+ public AlertReadFailureCounter? ReadFailures { get; set; }
+
public AlertEngine Build(bool withFailedJobsFetcher = false) => new(
Settings, Adapter, StateStore, Deliverer,
isAlertMuted: _ => Muted,
@@ -298,7 +303,8 @@ private sealed class Harness
: null,
resolutionCallback: (r, _) => { Resolutions.Add(r); return Task.CompletedTask; },
logger: null,
- utcNow: () => Now);
+ utcNow: () => Now,
+ readFailures: ReadFailures);
public static AlertServerSnapshot Snapshot(
double? sqlCpu = null, double? totalCpu = null,
@@ -1420,6 +1426,112 @@ public async Task FailedJobs_GatedOnOnlineAndNotAzure_AndSuppressionHoldsTheWate
Assert.Single(h.StateStore.SavedFailedJob);
}
+ /* ---------------- #3013: swallowed condition reads reach a counter ---------------- */
+
+ [Fact]
+ public async Task EverySwallowedConditionRead_LandsOnTheCounter_UnderTheServerItBelongsTo()
+ {
+ /* #3013's whole defect is that these skips reached no surface. The log-and-skip posture itself is
+ correct and is pinned by AdapterFailure_SkipsThatCheck_WithoutDisturbingItsState below; what this
+ pin adds is that the skip is now COUNTED, per server, with the failing read named.
+
+ Three checks enabled rather than all of them, because the exact total over an all-enabled sweep
+ depends on gates this pin is not about (Azure-ness, the wait-seconds opt-in, whether a fetcher was
+ supplied). Three is enough to prove the count is per-read and not per-pass. */
+ var counter = new AlertReadFailureCounter(() => new DateTime(2026, 9, 5, 8, 0, 0, DateTimeKind.Utc));
+ var h = new Harness { ReadFailures = counter };
+ h.Settings.BlockingEnabled = true;
+ h.Settings.DeadlockEnabled = true;
+ h.Settings.DatabaseStateEnabled = true;
+ h.Settings.ForcePlanFailureEnabled = true;
+
+ var engine = new AlertEngine(
+ h.Settings, new ThrowingAdapter(), h.StateStore, h.Deliverer, _ => false,
+ utcNow: () => h.Now, readFailures: counter);
+
+ await engine.EvaluateServerAsync(Harness.Snapshot());
+
+ var reading = counter.ReadFor(Key);
+
+ /* Blocking, deadlocks, database state and forced plans all threw. The watermark seed reads the same
+ throwing adapter and is counted too, which is deliberate: a failed seed means the edge triggers
+ start from nothing for that server, which is exactly the kind of silent degradation #3013 is
+ about. Bounded rather than exact so the pin does not have to be rewritten every time a check is
+ added, with the LOWER bound the part that carries the claim. */
+ Assert.True(
+ reading.ServerReadFailures >= 4,
+ $"expected at least the four enabled condition reads to be counted, got {reading.ServerReadFailures}");
+
+ /* The denominator, and the reason this is not just a count: one pass. */
+ Assert.Equal(1, reading.ServerAlertPasses);
+
+ /* Nothing leaked to another server or to the fleet bucket: the instance total is this server's. */
+ Assert.Equal(reading.ServerReadFailures, reading.InstanceReadFailures);
+ Assert.Equal(0, counter.ReadFor("999").ServerReadFailures);
+
+ /* The currency stamp and the named read — the two things a bare count cannot say. */
+ Assert.Equal(new DateTime(2026, 9, 5, 8, 0, 0, DateTimeKind.Utc), reading.LastFailureAtUtc);
+ Assert.False(string.IsNullOrWhiteSpace(reading.LastFailureRead));
+
+ /* The finding names the read rather than restating the number. */
+ var finding = AlertReadFailureCounter.FormatFinding(reading);
+ Assert.NotNull(finding);
+ Assert.Contains(reading.LastFailureRead!, finding, StringComparison.Ordinal);
+ }
+
+ [Fact]
+ public async Task AHealthyPass_CountsItselfAndLeavesTheFailureCountAtZero()
+ {
+ /* The control, and the half that decides whether the counter can be read as reassurance: a pass over
+ an adapter that answers normally must move the DENOMINATOR and nothing else. Without this, a
+ counter wired to increment on every pass would look identical to a working one on the test above.
+
+ The control also has to exercise the case worth worrying about — a pass that actually RAN its
+ checks — so the same four checks are enabled here as in the failing pin, against the harness's
+ own answering adapter rather than the throwing one. */
+ var counter = new AlertReadFailureCounter();
+ var h = new Harness { ReadFailures = counter };
+ h.Settings.BlockingEnabled = true;
+ h.Settings.DeadlockEnabled = true;
+ h.Settings.DatabaseStateEnabled = true;
+ h.Settings.ForcePlanFailureEnabled = true;
+ var engine = h.Build();
+
+ await engine.EvaluateServerAsync(Harness.Snapshot());
+ await engine.EvaluateServerAsync(Harness.Snapshot());
+
+ var reading = counter.ReadFor(Key);
+ Assert.Equal(2, reading.ServerAlertPasses);
+ Assert.Equal(0, reading.ServerReadFailures);
+ Assert.Equal(0, reading.InstanceReadFailures);
+ Assert.Null(reading.LastFailureAtUtc);
+ Assert.Null(reading.LastFailureRead);
+
+ /* A clean reading carries no sentence at all, rather than a sentence saying it is clean — the
+ #3017 discipline: a finding that always renders trains a reader to skip it. */
+ Assert.Null(AlertReadFailureCounter.FormatFinding(reading));
+
+ /* And the checks really did run against the adapter, so the zero above is a zero from a pass that
+ looked rather than one that was gated off. */
+ Assert.True(h.Adapter.ForcePlanFetches > 0, "the control pass performed no reads, so its zero proves nothing");
+ }
+
+ [Fact]
+ public async Task TheMasterSwitchOffPass_IsNotInTheDenominator()
+ {
+ /* A pass that never looked at the store must not dilute the denominator — otherwise a fleet with
+ alerts switched off accumulates passes forever and three failures over "50,000 passes" reads as
+ negligible when the real denominator is three. */
+ var counter = new AlertReadFailureCounter();
+ var h = new Harness { ReadFailures = counter };
+ h.Settings.AlertsEnabled = false;
+ var engine = h.Build();
+
+ await engine.EvaluateServerAsync(Harness.Snapshot());
+
+ Assert.Equal(0, counter.ReadFor(Key).ServerAlertPasses);
+ }
+
/* ---------------- engine hygiene ---------------- */
[Fact]
diff --git a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
new file mode 100644
index 000000000..45d012128
--- /dev/null
+++ b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
@@ -0,0 +1,613 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System;
+using System.Collections.Generic;
+using System.Globalization;
+using System.IO;
+using System.Linq;
+using System.Reflection;
+using System.Runtime.CompilerServices;
+using System.Text.RegularExpressions;
+using PerformanceMonitor.Alerting;
+using PerformanceMonitor.Common;
+using Xunit;
+
+namespace Darling.Tests;
+
+///
+/// #3013: every swallowed alerting-side store read is COUNTED, and the count reaches a surface.
+///
+/// The defect was not the log-and-skip — that is correct, and stays. It was that a skip 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 it, and the population it found was RISING (41 → 61 service-log
+/// errors per hour) over hours in which collector failures FELL (23 → 2), because the alert pass runs on
+/// a far shorter store deadline than the collection sweep.
+///
+/// Why the call-site census is a source scan and not a list. Thirty-odd catch blocks across
+/// three files swallow a read of the store on behalf of an alert. A pin that named them would restate
+/// today's answer; the one thing it has to do is notice the thirty-FIRST. So every
+/// catch (Exception …) block in the scoped regions is enumerated from source and each must be
+/// EITHER counted or explicitly exempt with a stated reason — a new block that is neither fails, quoting
+/// its own log line. That is the property a hand-written list cannot have (#3017's lesson, one level
+/// down), and the counts are asserted in both directions so a walk that silently stopped reaching cannot
+/// report clean.
+///
+public sealed class AlertReadFailureSurfaceTests
+{
+ /* ---------------- the counter's own behaviour ---------------- */
+
+ [Fact]
+ public void TheFleetBucket_IsHeldApartFromEveryServer()
+ {
+ /* The load-bearing separation. The four fleet-scoped store self-alerts (disk pressure,
+ compression-job health, store-job cadence, retention holds) belong to no server, so if they
+ landed in a per-server bucket they would be attributed to whichever key was handy, and if they
+ landed nowhere they would be exactly as invisible as #3013 found the whole class. They land in
+ the instance total and in no server's count. */
+ var counter = new AlertReadFailureCounter();
+
+ counter.RecordReadFailure("101", "deadlocks");
+ counter.RecordReadFailure("202", "blocking");
+ counter.RecordReadFailure(null, "store background-job health reads");
+
+ Assert.Equal(1, counter.ReadFor("101").ServerReadFailures);
+ Assert.Equal(1, counter.ReadFor("202").ServerReadFailures);
+ Assert.Equal(3, counter.ReadFor("101").InstanceReadFailures);
+
+ /* No server key can reach the fleet bucket, whatever it is spelled — the reason the bucket is a
+ separate field rather than a sentinel key in the map. */
+ foreach (var spelling in new[] { "", " ", "null", "(fleet)", "0", "-1" })
+ {
+ Assert.Equal(0, counter.ReadFor(spelling).ServerReadFailures);
+ }
+
+ Assert.DoesNotContain(string.Empty, counter.ServerKeys());
+ Assert.Equal(new[] { "101", "202" }, counter.ServerKeys());
+
+ /* And the instance-wide read sees it, so a caller with no server in hand is not blind to it. */
+ var (instanceFailures, instanceStamp, instanceRead) = counter.ReadInstance();
+ Assert.Equal(3, instanceFailures);
+ Assert.NotNull(instanceStamp);
+ Assert.Equal("store background-job health reads", instanceRead);
+ }
+
+ [Fact]
+ public void AnUnseenServer_ReadsAsZeroesAndNotAsAnAbsence()
+ {
+ /* The surface serializes this straight into JSON, so a null-shaped reading for a server that has
+ simply never failed would render as a block of nulls that an operator has to interpret. Zero
+ with a counting_since stamp is a statement; null is a question. */
+ var started = new DateTime(2026, 9, 5, 1, 2, 3, DateTimeKind.Utc);
+ var counter = new AlertReadFailureCounter(() => started);
+
+ var reading = counter.ReadFor("never-seen");
+
+ Assert.Equal(0, reading.ServerReadFailures);
+ Assert.Equal(0, reading.ServerAlertPasses);
+ Assert.Equal(0, reading.InstanceReadFailures);
+ Assert.Null(reading.LastFailureAtUtc);
+ Assert.Null(reading.LastFailureRead);
+ Assert.Equal(started, reading.CountingSinceUtc);
+ }
+
+ [Fact]
+ public void AServerWithNoFailuresInsideADegradedService_SaysBothThings()
+ {
+ /* The arm that exists because of the fleet-scoped conditions. A server whose own alert reads are
+ all fine, on a service whose store self-alerts cannot read at all, must not render a bare
+ "0 failures" — that is true about the server and misleading about the instance the operator is
+ standing on. */
+ var counter = new AlertReadFailureCounter();
+ counter.RecordReadFailure(null, "store background-job health reads");
+ counter.RecordReadFailure("999", "deadlocks");
+
+ var reading = counter.ReadFor("101");
+ Assert.Equal(0, reading.ServerReadFailures);
+ Assert.Equal(2, reading.InstanceReadFailures);
+
+ var finding = AlertReadFailureCounter.FormatFinding(reading);
+ Assert.NotNull(finding);
+ Assert.Contains("No alerting-side store read has failed for this server", finding, StringComparison.Ordinal);
+ Assert.Contains("2 failed elsewhere", finding, StringComparison.Ordinal);
+ }
+
+ [Fact]
+ public void AnUnnamedRead_StillCountsAndStillNamesSomething()
+ {
+ /* A future call site that passes an empty name must not produce a finding whose sentence trails
+ off into nothing. It counts, and it says so with a placeholder rather than silently. */
+ var counter = new AlertReadFailureCounter();
+ counter.RecordReadFailure("101", " ");
+
+ var reading = counter.ReadFor("101");
+ Assert.Equal(1, reading.ServerReadFailures);
+ Assert.Equal("unnamed read", reading.LastFailureRead);
+ }
+
+ [Fact]
+ public void TheWindowNote_NamesTheWindowItMeasuredAndDisclaimsTheOneItDidNot()
+ {
+ /* The whole response is the trailing seven days except this block, and a reader who assumed
+ otherwise reads a zero as seven quiet days when a restart a minute ago is all it means. The note
+ has to say so in both directions — what it IS, and what it is NOT — which is #3017's
+ output_note discipline applied to a different window. It also has to refuse the OTHER
+ misreading: that a zero here says anything about alert DELIVERY. */
+ var note = AlertReadFailureCounter.WindowNote;
+
+ foreach (var phrase in new[]
+ {
+ "not measured over the trailing seven", /* the disclaimed window, named */
+ "counting_since", /* the floor under the zero */
+ "restart takes it to zero", /* why the zero can be small */
+ "deliberately not persisted", /* and why it is in memory */
+ "failed to DELIVER", /* the claim it refuses to make */
+ "fleet-scoped store self-alerts", /* what the instance total covers */
+ "not a rate", /* what the denominator is not */
+ })
+ {
+ Assert.Contains(phrase, note, StringComparison.Ordinal);
+ }
+
+ /* The control for the phrase list above: the identical Contains form finds a planted string that
+ IS present and does not find one that is not, so its silence on a missing phrase would be a
+ real failure rather than a matcher that never matches anything. */
+ Assert.Contains("alert_read_health", note, StringComparison.Ordinal);
+ Assert.DoesNotContain("trailing seven days is the window for this block", note, StringComparison.Ordinal);
+ }
+
+ /* ---------------- the call-site census ---------------- */
+
+ ///
+ /// Files swept WHOLE, because every catch (Exception …) in them belongs to the alerting layer.
+ ///
+ private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScopes =
+ {
+ (Path.Combine("PerformanceMonitor.Alerting", "AlertEngine.cs"), 14, 3),
+ (Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingSelfAlertEvaluator.cs"), 5, 7),
+ };
+
+ ///
+ /// DarlingWorker.cs holds forty-odd catch blocks across many regimes, so it is scoped to the
+ /// members that perform alerting reads. Named rather than derived by a call-graph walk because these
+ /// are the ENTRY points of independent passes rather than one pass's closure — nothing calls them but
+ /// the sweep loop — and a walk out of the loop body would reach the collection sweep and the command
+ /// plane with it.
+ ///
+ /// The count below is the tripwire that makes the list safe: a member added to this file with an
+ /// alerting read in it does not silently inherit clean status, because the totals asserted over the
+ /// scope stop matching the moment its catch blocks are neither counted nor exempt.
+ ///
+ private static readonly string[] s_workerAlertMembers =
+ {
+ "EvaluateAlertsAsync",
+ "EvaluatePostgresAlertsAsync",
+ "EvaluatePgCpuAsync",
+ "EvaluatePgDeadlocksAsync",
+ "EvaluatePgBlockingAsync",
+ "EvaluatePgLongRunningQueryAsync",
+ "EvaluatePgPoisonWaitAsync",
+ "EvaluateCompressionJobHealthAsync",
+ "EvaluateStoreDiskPressureAsync",
+ "ReadStoreSizeBytesAsync",
+ "SweepStoreSelfMetricsAsync",
+ "NotifyPgResolutionAsync",
+ "FetchFailedJobsAsync",
+ };
+
+ private const int WorkerCountedSites = 8;
+ private const int WorkerExemptSites = 5;
+
+ ///
+ /// Log-message fragments that identify a catch block DELIBERATELY not counted, each paired with the
+ /// reason. Keyed on the message because that is the one part of a catch block that names what it was
+ /// handling; the source itself carries the same reason as a comment at the site.
+ ///
+ private static readonly Dictionary s_exemptions = new(StringComparer.Ordinal)
+ {
+ ["Could not load incident occurrences"] = "bookkeeping about an alert, not the condition read it is judged on",
+ ["Could not persist incident occurrences"] = "a write",
+ ["Alert resolution callback failed"] = "the delivery path",
+ ["Connection-change self-alert delivery failed"] = "the delivery path",
+ ["Store disk-pressure self-alert failed"] = "handed its evidence as parameters; the read is counted in DarlingWorker",
+ ["Store runtime upgrade self-alert failed"] = "handed its evidence as parameters",
+ ["Compression-job health self-alert failed"] = "handed its evidence as parameters; the read is counted in DarlingWorker",
+ ["Store-job cadence self-alert failed"] = "handed its evidence as parameters; the read is counted in DarlingWorker",
+ ["Retention-held self-alert failed"] = "handed its evidence as parameters; the read is counted in DarlingWorker",
+ ["Failed to record resolution"] = "an audit-row write",
+ ["Could not record Postgres alert resolution"] = "a history write",
+ ["could not read the store volume free space"] = "a local filesystem read, not a store read",
+ ["could not read pg_database_size"] = "context for the alert text, not the evidence the alert is judged on",
+ ["Store self-metrics sweep did not finish"] = "a metrics write sweep; no alert is judged on its result",
+ ["Recently-failed-job check errored"] = "reads the monitored server's msdb on its own connection and timeout",
+ };
+
+ private static readonly Regex s_catch = new(
+ @"catch\s*\(\s*(?:System\s*\.\s*)?Exception\b",
+ RegexOptions.Compiled | RegexOptions.CultureInvariant);
+
+ [Fact]
+ public void EverySwallowedAlertingRead_IsCountedOrExplicitlyExempt()
+ {
+ var unclassified = new List();
+ var totalCounted = 0;
+ var totalExempt = 0;
+
+ foreach (var (relative, expectedCounted, expectedExempt) in s_wholeFileScopes)
+ {
+ var raw = ReadSource(relative);
+ var (counted, exempt) = Classify(
+ raw, CSharpSourceWalker.StripCommentsAndStrings(raw), relative, unclassified);
+
+ Assert.Equal(expectedCounted, counted);
+ Assert.Equal(expectedExempt, exempt);
+
+ totalCounted += counted;
+ totalExempt += exempt;
+ }
+
+ var workerRaw = ReadSource(Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"));
+ var workerStripped = CSharpSourceWalker.StripCommentsAndStrings(workerRaw);
+ var workerCounted = 0;
+ var workerExempt = 0;
+
+ foreach (var member in s_workerAlertMembers)
+ {
+ var (start, end) = MemberBody(workerStripped, member);
+ var (counted, exempt) = Classify(
+ workerRaw[start..end], workerStripped[start..end], $"DarlingWorker.{member}", unclassified);
+ workerCounted += counted;
+ workerExempt += exempt;
+ }
+
+ /* Offenders BEFORE the census, so a genuinely unclassified block reports as itself rather than as
+ an off-by-one on a total. */
+ Assert.True(
+ unclassified.Count == 0,
+ $"{unclassified.Count} alerting catch block(s) neither count a swallowed read nor carry a stated "
+ + "exemption. Either add the RecordReadFailure call or add the message to s_exemptions with a "
+ + $"reason: {string.Join(" | ", unclassified)}");
+
+ Assert.Equal(WorkerCountedSites, workerCounted);
+ Assert.Equal(WorkerExemptSites, workerExempt);
+
+ totalCounted += workerCounted;
+ totalExempt += workerExempt;
+
+ /* The whole-tree totals, so a site MOVED between the scoped regions still has to be re-counted by
+ a person rather than netting out silently. */
+ Assert.Equal(27, totalCounted);
+ Assert.Equal(15, totalExempt);
+
+ /* Every exemption in the table is actually used. An exemption for a message that no longer exists
+ is a hole this pin would otherwise keep open indefinitely — the shape that lets a real new catch
+ block match a stale entry by accident. */
+ Assert.Equal(s_exemptions.Count, totalExempt);
+ }
+
+ [Fact]
+ public void TheScanner_FindsAPlantedCatchBlockAndRejectsAPlantedProseOne()
+ {
+ /* The positive control for the census above, run through the IDENTICAL Classify call. Without it
+ the scan could match nothing and report clean, which is exactly how a source-scanning guard
+ starts lying. Three fixtures: a counted block, an exempt block, and a block that is neither —
+ the third asserting the scan FAILS when it should. */
+ var unclassified = new List();
+
+ const string countedFixture = """
+ try { Read(); }
+ catch (Exception ex)
+ {
+ _logger?.LogError("Failed to check widgets for {Server}: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(key, "widgets");
+ }
+ """;
+ var (counted, exempt) = Classify(countedFixture, CSharpSourceWalker.StripCommentsAndStrings(countedFixture), "fixture", unclassified);
+ Assert.Equal(1, counted);
+ Assert.Equal(0, exempt);
+ Assert.Empty(unclassified);
+
+ const string exemptFixture = """
+ try { Write(); }
+ catch (Exception ex)
+ {
+ _logger?.LogError("Alert resolution callback failed for {Server}: {Message}", serverName, ex.Message);
+ }
+ """;
+ (counted, exempt) = Classify(exemptFixture, CSharpSourceWalker.StripCommentsAndStrings(exemptFixture), "fixture", unclassified);
+ Assert.Equal(0, counted);
+ Assert.Equal(1, exempt);
+ Assert.Empty(unclassified);
+
+ const string strayFixture = """
+ try { Read(); }
+ catch (Exception ex)
+ {
+ _logger?.LogError("Failed to check sprockets for {Server}: {Message}", serverName, ex.Message);
+ }
+ """;
+ (counted, exempt) = Classify(strayFixture, CSharpSourceWalker.StripCommentsAndStrings(strayFixture), "fixture", unclassified);
+ Assert.Equal(0, counted);
+ Assert.Equal(0, exempt);
+ Assert.Single(unclassified);
+ Assert.Contains("sprockets", unclassified[0], StringComparison.Ordinal);
+
+ /* And a catch written only in PROSE is not a catch. The census reads stripped source for exactly
+ this reason — the exemption comments this change added to fourteen sites are prose, and a
+ scanner that counted them would have inflated every total. */
+ var prose = CSharpSourceWalker.StripCommentsAndStrings("""
+ /* catch (Exception ex) — this comment is not a catch block. */
+ var x = 1;
+ """);
+ Assert.Empty(s_catch.Matches(prose));
+ }
+
+ [Fact]
+ public void EveryCountedSite_NamesItsReadDistinctly()
+ {
+ /* A name is the actionable half of the count — which condition went blind, not merely that one
+ did — so two sites sharing a name would make last_failure_read ambiguous exactly when it is
+ being read in anger. Reflected off source across all three files rather than listed, so a
+ copy-pasted call site fails here instead of shipping. */
+ var names = new List<(string Name, string Where)>();
+
+ foreach (var relative in new[]
+ {
+ Path.Combine("PerformanceMonitor.Alerting", "AlertEngine.cs"),
+ Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingSelfAlertEvaluator.cs"),
+ Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"),
+ })
+ {
+ var raw = ReadSource(relative);
+ foreach (Match m in Regex.Matches(raw, @"RecordReadFailure\([^,]+,\s*""([^""]+)""\s*\)"))
+ {
+ names.Add((m.Groups[1].Value, relative));
+ }
+ }
+
+ Assert.Equal(27, names.Count);
+ Assert.All(names, n => Assert.False(string.IsNullOrWhiteSpace(n.Name)));
+
+ var duplicates = names
+ .GroupBy(n => n.Name, StringComparer.Ordinal)
+ .Where(g => g.Count() > 1)
+ .Select(g => g.Key)
+ .ToList();
+
+ Assert.True(duplicates.Count == 0, $"duplicate read name(s): {string.Join(", ", duplicates)}");
+ }
+
+ /* ---------------- the surfaces ---------------- */
+
+ [Fact]
+ public void TheDarlingSurface_DerivesTheSameServerKeyAsTheDarlingAlertPass()
+ {
+ /* The silent-zero hazard. The counter is keyed by the alert pass's own server key, ordinal, so a
+ reader that rendered the key differently would look up a bucket nothing ever wrote and report a
+ confident zero — the failure mode this whole change exists to remove, reintroduced by the fix.
+ Both sides go through int.ToString(CultureInfo.InvariantCulture) on Darling; this pins that they
+ are the SAME expression rather than trusting two files to stay in step. */
+ var tool = ReadSource(Path.Combine(
+ "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingMcpDataTools.cs"));
+ var worker = ReadSource(Path.Combine(
+ "Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"));
+
+ const string invariant = "ServerId.ToString(CultureInfo.InvariantCulture)";
+
+ Assert.Contains("AlertReadFailureCounter.Shared.ReadFor(", tool, StringComparison.Ordinal);
+ Assert.Contains("resolved." + invariant, tool, StringComparison.Ordinal);
+ Assert.Contains("runtime." + invariant, worker, StringComparison.Ordinal);
+
+ /* The self-alert half renders the same key through its own helper, so pin the helper rather than
+ its call sites. */
+ var evaluator = ReadSource(Path.Combine(
+ "Darling", "PerformanceMonitor.Darling.Service", "DarlingSelfAlertEvaluator.cs"));
+ Assert.Contains(
+ "private static string Key(int serverId) => serverId.ToString(CultureInfo.InvariantCulture);",
+ evaluator,
+ StringComparison.Ordinal);
+
+ /* The control: the same Contains form finds a deliberately WRONG spelling nowhere, so its silence
+ above is a real absence and not a matcher that never matches. */
+ Assert.DoesNotContain("ServerId.ToString(CultureInfo.CurrentCulture)", tool, StringComparison.Ordinal);
+ }
+
+ [Fact]
+ public void TheDarlingSurface_CarriesEveryFieldOfTheReading()
+ {
+ /* A field on the reading that no surface renders is a measurement nobody can act on — the #1837
+ relationship the web columns already document. Derived from the RECORD rather than listed, so a
+ field added to Reading fails here until a surface renders it. */
+ var tool = ReadSource(Path.Combine(
+ "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingMcpDataTools.cs"));
+
+ var block = ExtractAlertReadBlock(tool);
+
+ var readingMembers = typeof(AlertReadFailureCounter.Reading)
+ .GetProperties(BindingFlags.Public | BindingFlags.Instance)
+ .Select(p => p.Name)
+ .Where(n => n != "EqualityContract")
+ .ToList();
+
+ Assert.Equal(6, readingMembers.Count);
+
+ foreach (var member in readingMembers)
+ {
+ Assert.Contains("alertReads." + member, block, StringComparison.Ordinal);
+ }
+
+ /* Plus the two composed values, which are not on the record. */
+ Assert.Contains("finding = alertReadFinding", block, StringComparison.Ordinal);
+ Assert.Contains("note = AlertReadFailureCounter.WindowNote", block, StringComparison.Ordinal);
+ }
+
+ [Fact]
+ public void TheWebPanel_IsOnBothServerTabsAndSaysWhichWindowItIs()
+ {
+ /* The fourth surface (#3017 found it): the web dashboard renders exactly what its descriptors list,
+ so a field added to the tool and not to a descriptor is silently dropped. Two tabs share the
+ Collection Health fanout, so a panel added to one and not the other is the same drop on half the
+ fleet — pinned by COUNT, not by presence, which is the difference between this pin and the one
+ that would have passed with a single tab covered.
+ And the subtitle is load-bearing: this panel's figures are NOT the trailing seven days every
+ sibling panel on the tab is, so inheriting that subtitle would make the panel assert a window it
+ never measured. */
+ var js = ReadSource(Path.Combine(
+ "Darling", "PerformanceMonitor.Darling.Service", "wwwroot", "js", "pages", "server-tabs.js"));
+
+ var panelUses = Regex.Matches(js, @"^\s*ALERT_READ_PANEL,\s*$", RegexOptions.Multiline).Count;
+ var sweepUses = Regex.Matches(js, @"stats: SWEEP_STATS \},\s*$", RegexOptions.Multiline).Count;
+
+ Assert.Equal(sweepUses, panelUses);
+ Assert.Equal(2, panelUses);
+
+ Assert.Contains("subtitle: \"since this service started", js, StringComparison.Ordinal);
+ Assert.Contains("NOT the trailing 7 days", js, StringComparison.Ordinal);
+
+ foreach (var key in new[]
+ {
+ "alert_read_health.server_read_failures",
+ "alert_read_health.server_alert_passes",
+ "alert_read_health.instance_read_failures",
+ "alert_read_health.last_failure_read",
+ "alert_read_health.last_failure_at",
+ "alert_read_health.counting_since",
+ })
+ {
+ Assert.Contains(key, js, StringComparison.Ordinal);
+ }
+ }
+
+ [Fact]
+ public void TheBandingSignature_TakesNoAlertReadTerm()
+ {
+ /* #3017 kept the collector band free of its output figures because a verdict keyed on them fired
+ on the healthy quiet install. The same argument is stronger here: a band over blind alert reads
+ would have to guess how many make alerting unhealthy, and on THIS surface a wrong guess fails by
+ saying nothing is wrong. Read off the type so a tenth parameter fails rather than being
+ discovered later. */
+ var classify = typeof(CollectorHealthClassifier)
+ .GetMethod("Classify", BindingFlags.Public | BindingFlags.Static);
+
+ Assert.NotNull(classify);
+ Assert.Equal(9, classify!.GetParameters().Length);
+ Assert.DoesNotContain(
+ "alert",
+ string.Join("|", classify.GetParameters().Select(p => p.Name)),
+ StringComparison.OrdinalIgnoreCase);
+ }
+
+ /* ---------------- helpers ---------------- */
+
+ ///
+ /// Classifies every catch (Exception …) block in one span. Blocks are found in STRIPPED source
+ /// (so prose and literals cannot register as one) and their MESSAGES are read from the raw span at the
+ /// same offsets, which guarantees line up
+ /// because it preserves length.
+ ///
+ private static (int Counted, int Exempt) Classify(
+ string raw, string stripped, string where, List unclassified)
+ {
+ var counted = 0;
+ var exempt = 0;
+
+ foreach (Match m in s_catch.Matches(stripped))
+ {
+ var open = stripped.IndexOf('{', m.Index);
+ if (open < 0)
+ {
+ continue;
+ }
+
+ var body = CSharpSourceWalker.BraceBalanced(stripped, open);
+ var rawBody = raw[open..(open + body.Length)];
+
+ if (body.Contains("RecordReadFailure(", StringComparison.Ordinal))
+ {
+ counted++;
+ continue;
+ }
+
+ var match = s_exemptions.Keys.FirstOrDefault(k => rawBody.Contains(k, StringComparison.Ordinal));
+ if (match != null)
+ {
+ exempt++;
+ continue;
+ }
+
+ var firstLog = Regex.Match(rawBody, @"""([^""]{0,120})""");
+ unclassified.Add(
+ $"{where} @offset {open}: {(firstLog.Success ? firstLog.Groups[1].Value : rawBody.Trim())}");
+ }
+
+ return (counted, exempt);
+ }
+
+ ///
+ /// The brace-balanced body of one named member, over stripped source. Fails loudly when the member is
+ /// gone, because a rename that silently shrank the scope is how this kind of guard starts reporting
+ /// clean on code it no longer reads.
+ ///
+ private static (int Start, int End) MemberBody(string stripped, string member)
+ {
+ /* Matched as a DECLARATION LINE — an access modifier at the start of the line, then anything but
+ a newline or an assignment, then the name and its parameter list. Keyed on the modifier rather
+ than on the return type because the return types here include nested generics
+ (Task<List<FailedJobInfo>>), which a bracket-balanced return-type pattern silently fails to
+ match — and a silent non-match here reads as "member renamed" rather than as a broken regex. */
+ var decl = Regex.Match(
+ stripped,
+ @"^[ \t]*(?:private|internal|public|protected)[^\r\n=]*?\b" + Regex.Escape(member) + @"\s*\(",
+ RegexOptions.Multiline);
+ Assert.True(decl.Success, $"DarlingWorker member {member} has no declaration — a rename has moved it out from under this guard");
+
+ var open = stripped.IndexOf('{', decl.Index);
+ Assert.True(open > 0, $"DarlingWorker member {member} has no block body");
+
+ var body = CSharpSourceWalker.BraceBalanced(stripped, open);
+ return (open, open + body.Length);
+ }
+
+ /// The alert_read_health = new { … } initializer, from the tool's source.
+ private static string ExtractAlertReadBlock(string source)
+ {
+ var stripped = CSharpSourceWalker.StripCommentsAndStrings(source);
+ var at = stripped.IndexOf("alert_read_health = new", StringComparison.Ordinal);
+ Assert.True(at > 0, "the tool no longer builds an alert_read_health block");
+
+ var open = stripped.IndexOf('{', at);
+ Assert.True(open > 0, "alert_read_health has no initializer");
+
+ var body = CSharpSourceWalker.BraceBalanced(stripped, open);
+ return source[open..(open + body.Length)];
+ }
+
+ private static string ReadSource(string relative)
+ {
+ var path = Path.Combine(RepoRoot(), relative);
+
+ Assert.True(File.Exists(path), $"#3013 scan target not found: {path}");
+
+ return File.ReadAllText(path);
+ }
+
+ private static string RepoRoot([CallerFilePath] string thisFile = "")
+ {
+ var dir = Path.GetDirectoryName(thisFile)!;
+ while (dir is not null
+ && !File.Exists(Path.Combine(dir, "PerformanceMonitor.sln"))
+ && !Directory.Exists(Path.Combine(dir, ".git")))
+ {
+ dir = Path.GetDirectoryName(dir);
+ }
+
+ Assert.NotNull(dir);
+ return dir!;
+ }
+}
diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs
index 1b73bd5b5..4e2c6f977 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs
@@ -359,7 +359,8 @@ public DarlingSelfAlertEvaluator(
Func? agLagAlertSeconds = null,
Func? agRedoQueueAlertKb = null,
Func? agDisconnectRefireMinutes = null,
- Func? storeJobCadenceWarnPercent = null)
+ Func? storeJobCadenceWarnPercent = null,
+ AlertReadFailureCounter? readFailures = null)
{
_settings = settings ?? throw new ArgumentNullException(nameof(settings));
_deliverer = deliverer ?? throw new ArgumentNullException(nameof(deliverer));
@@ -379,8 +380,17 @@ public DarlingSelfAlertEvaluator(
/* Unsupplied falls back to the V57 DDL default, so an evaluator built without the seam behaves
like a store at its shipped defaults (the AG-seam discipline). */
_storeJobCadenceWarnPercent = storeJobCadenceWarnPercent ?? (() => 25);
+ _readFailures = readFailures;
}
+ ///
+ /// Where a SWALLOWED self-alert store read is counted (#3013), or null when nothing is counting.
+ /// Only the conditions that READ the store here increment it; the fleet-scoped conditions are handed
+ /// their evidence as parameters, so their reads are counted at their own sites in
+ /// DarlingWorker — see the exemption notes at each catch.
+ ///
+ private readonly AlertReadFailureCounter? _readFailures;
+
private enum ConnectionState
{
/* Never yet observed — the baseline. Unknown→online/offline never fires (mirrors the
@@ -408,6 +418,10 @@ public async Task EvaluateStoreAlertsAsync(
return;
}
+ /* #3013: this server's second alert pass of the sweep, counted after the master-switch return so a
+ pass that never looked at the store is not in the denominator. */
+ _readFailures?.RecordPass(Key(serverId));
+
/* Only judge collection-stopped once the service has actually collected from this server this run
(see _hasBeenOnline) — otherwise pre-restart / pre-re-add stale rows would false-alarm before the
first fresh collection lands. */
@@ -431,6 +445,7 @@ only as the shipped defaults. */
catch (Exception ex)
{
_logger?.LogError("[{Server}] Collection-health self-alert failed: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(Key(serverId), "collection-health self-alert");
}
}
@@ -451,6 +466,7 @@ only as the shipped defaults. */
catch (Exception ex)
{
_logger?.LogError("[{Server}] Capture-down self-alert failed: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(Key(serverId), "capture-down self-alert");
}
try
@@ -492,6 +508,7 @@ otherwise ask the collected history once and memoize the positive. */
catch (Exception ex)
{
_logger?.LogError("[{Server}] Agent-not-running self-alert failed: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(Key(serverId), "agent-not-running self-alert");
}
/* Availability Group health (#991). Skipped entirely when the master AG switch is off, so a fleet that
@@ -527,6 +544,7 @@ otherwise ask the collected history once and memoize the positive. */
catch (Exception ex)
{
_logger?.LogError("[{Server}] Availability-Group self-alert failed: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(Key(serverId), "Availability-Group self-alert");
}
}
@@ -682,6 +700,7 @@ public async Task EvaluateCollectorCostAsync(NpgsqlDataSource postgres, Cancella
{
/* A self-alert read that fails must not take the loop down — it is best-effort telemetry. */
_logger?.LogDebug(ex, "collector-cost regression evaluation failed");
+ _readFailures?.RecordReadFailure(null, "collector-cost regression self-alert");
return;
}
@@ -942,6 +961,7 @@ was never a measurement of this server's reachability. */
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: the DELIVERY path, not a condition read. */
_logger?.LogError("[{Server}] Connection-change self-alert delivery failed: {Message}", serverName, ex.Message);
}
}
@@ -1359,6 +1379,9 @@ public async Task EvaluateDiskPressureAsync(
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: this method is handed its evidence as parameters and
+ performs no store read — the catch covers the apply/deliver half. The reads that FEED it are counted
+ at their own sites in DarlingWorker. */
_logger?.LogError("Store disk-pressure self-alert failed: {Message}", ex.Message);
}
}
@@ -1557,6 +1580,7 @@ await FireAsync(
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: the report is a parameter; no store read happens here. */
_logger?.LogError("Store runtime upgrade self-alert failed: {Message}", ex.Message);
}
}
@@ -1583,6 +1607,8 @@ public async Task EvaluateCompressionJobsAsync(
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: the stuck-job list is a parameter; the read that
+ produces it is counted in DarlingWorker.EvaluateCompressionJobHealthAsync. */
_logger?.LogError("Compression-job health self-alert failed: {Message}", ex.Message);
}
}
@@ -1605,6 +1631,8 @@ public async Task EvaluateStoreJobCadenceAsync(
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: the readings are a parameter; the read that produces
+ them is counted in DarlingWorker.EvaluateCompressionJobHealthAsync. */
_logger?.LogError("Store-job cadence self-alert failed: {Message}", ex.Message);
}
}
@@ -1701,6 +1729,8 @@ public async Task EvaluateRetentionHoldsAsync(
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: the policies are a parameter; the read that produces
+ them is counted in DarlingWorker.EvaluateCompressionJobHealthAsync. */
_logger?.LogError("Retention-held self-alert failed: {Message}", ex.Message);
}
}
@@ -2350,6 +2380,7 @@ private async Task RecordResolutionAsync(AlertResolution resolution, Cancellatio
{
/* An audit-row write must never break the loop (RecordAlertAsync is already failure-isolated;
this is belt-and-suspenders for any other IAlertHistoryStore). */
+ /* NOT counted by #3013's swallowed-read counter: an audit-row WRITE, not a condition read. */
_logger?.LogError("Failed to record resolution '{Title}': {Message}", resolution.Title, ex.Message);
}
}
diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
index d10a0dce4..3b918893c 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
@@ -544,6 +544,12 @@ without a restart. */
encrypted_password SELECT-carve fails that whole read). */
private readonly MonitoredServerRegistryState _registryState;
+ /// #3013: the process counter this worker's own swallowed alert reads are tallied on —
+ /// the alert pass entry point, the six PostgreSQL predictor passes, and the store background-job
+ /// health reads behind the fleet-scoped self-alerts. The same instance the engine and the
+ /// self-alert evaluator are constructed with.
+ private readonly AlertReadFailureCounter _readFailures = AlertReadFailureCounter.Shared;
+
/* #2953: the collector's own startup verdict, published to the web host so /api/ping can report whether
collection is actually running WITHOUT reading the store. The other three seams carry control-plane
values the store is the authority for; this one carries the one fact the store cannot be asked about,
@@ -1472,7 +1478,10 @@ without a restart (and the clamps live on the settings properties, not here). */
agRedoQueueAlertKb: () => alertSettings.AgRedoQueueAlertKb,
agDisconnectRefireMinutes: () => alertSettings.AgDisconnectRefireMinutes,
/* #2136: the cadence warning threshold, read live like the AG seams (clamped on the property). */
- storeJobCadenceWarnPercent: () => alertSettings.StoreJobCadenceWarnPercent);
+ storeJobCadenceWarnPercent: () => alertSettings.StoreJobCadenceWarnPercent,
+ /* #3013: the same process counter the shared engine tallies on, so one number covers both
+ halves of a server's alert work. */
+ readFailures: AlertReadFailureCounter.Shared);
/* #1706: report this start's store runtime upgrade, now that there IS an alert engine to report it
through. Fired once, here, and never re-evaluated — the store is down while an upgrade runs, so
@@ -3141,7 +3150,10 @@ a control-plane reload reaches the very next check. */
never break the sweep. */
await historyStore.RecordAlertAsync(DarlingSelfAlertEvaluator.BuildResolutionRecord(resolution));
},
- logger: _logger);
+ logger: _logger,
+ /* #3013: the process counter every swallowed condition read is tallied on. Passed explicitly
+ rather than defaulted inside the engine so a test constructs its own and cannot pollute it. */
+ readFailures: AlertReadFailureCounter.Shared);
}
///
@@ -3192,6 +3204,7 @@ PostgreSQL read. */
catch (Exception ex)
{
_logger.LogError("[{Server}] Alert sweep failed: {Message}", server.Config.DisplayName, ex.Message);
+ _readFailures?.RecordReadFailure(runtime.ServerId.ToString(CultureInfo.InvariantCulture), "alert pass (latest-CPU read and the shared engine sweep)");
}
}
@@ -3307,6 +3320,7 @@ different subject does not. */
{
_logger.LogError("[{Server}] PostgreSQL alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
+ _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL outage-predictor reads");
}
/* #2711/#2719: Deadlocks, Blocking, Long-Running Query, Poison Wait and High CPU, each
@@ -3408,6 +3422,7 @@ reading is null
{
_logger.LogError("[{Server}] PostgreSQL CPU alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
+ _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL CPU alert read");
}
}
@@ -3521,6 +3536,7 @@ await NotifyPgResolutionAsync(key, snapshot.ServerName, metricName, "Deadlocks C
{
_logger.LogError("[{Server}] PostgreSQL deadlock alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
+ _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL deadlock alert read");
}
}
@@ -3631,6 +3647,7 @@ await NotifyPgResolutionAsync(key, snapshot.ServerName, metricName, "Blocking Cl
{
_logger.LogError("[{Server}] PostgreSQL blocking alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
+ _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL blocking alert read");
}
}
@@ -3747,6 +3764,7 @@ await NotifyPgResolutionAsync(key, snapshot.ServerName, metricName, "Long-Runnin
{
_logger.LogError("[{Server}] PostgreSQL long-running-query alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
+ _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL long-running-query alert read");
}
}
@@ -3949,6 +3967,7 @@ await NotifyPgResolutionAsync(serverKey, snapshot.ServerName, metricName, "Poiso
{
_logger.LogError("[{Server}] PostgreSQL poison wait alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
+ _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL poison wait alert read");
}
}
@@ -3982,6 +4001,7 @@ await _historyStore.RecordAlertAsync(DarlingSelfAlertEvaluator.BuildResolutionRe
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: a history WRITE, not a condition read. */
_logger.LogWarning("Could not record Postgres alert resolution for {Server}/{Metric}: {Message}",
serverName, metricName, ex.Message);
}
@@ -4224,6 +4244,8 @@ private async Task EvaluateStoreDiskPressureAsync(DarlingConfig config, Cancella
catch (Exception ex) when (ex is not OperationCanceledException)
{
/* Best-effort: an unreadable drive just means no disk signal this tick. */
+ /* NOT counted by #3013's swallowed-read counter: a local filesystem read, not a store read. #3013's
+ mechanism is store latency crossing the alert pass's deadline, which has no bearing on DriveInfo. */
_logger.LogDebug("Store disk-pressure check: could not read the store volume free space: {Message}", ex.Message);
}
}
@@ -4251,6 +4273,9 @@ private async Task EvaluateStoreDiskPressureAsync(DarlingConfig config, Cancella
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
+ /* NOT counted by #3013's swallowed-read counter: this read is CONTEXT for the alert text, not the
+ evidence the alert is judged on - that is freeBytes/totalBytes above. Losing it costs the message a
+ number; it does not make the condition unjudgeable. */
_logger.LogDebug("Store disk-pressure check: could not read pg_database_size: {Message}", ex.Message);
return null;
}
@@ -4314,6 +4339,7 @@ Judged on the CONSEQUENCE (held AND the tier past its own horizon), never on the
catch (Exception ex)
{
_logger.LogError("Compression-job health check failed: {Message}", ex.Message);
+ _readFailures?.RecordReadFailure(null, "store background-job health reads (compression, job cadence, retention holds)");
}
}
@@ -4359,6 +4385,9 @@ could take against a genuinely wedged store. The per-statement CommandTimeout in
self-heal on the next tick. The budget CTS surfaces as OperationCanceledException — with
the SERVICE token untripped that can only be the sweep budget, so it takes the timeout
arm too. */
+ /* NOT counted by #3013's swallowed-read counter, in either arm: this sweep WRITES the
+ self-metrics series. No alert is judged on its result — the store self-alerts read their own
+ evidence in EvaluateCompressionJobHealthAsync, which IS counted. */
if (PgBaselineProvider.IsCommandTimeout(ex) || (ex is OperationCanceledException && budget.IsCancellationRequested))
{
_logger.LogError(
@@ -4898,6 +4927,10 @@ box had the role and still landed here every cycle. */
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: this 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 #3013's mechanism can produce, and pooling the two would put a target-side outage
+ in a number the operator reads as store contention. */
_logger.LogWarning("[{Server}] Recently-failed-job check errored: {Message}",
runtime.Config.DisplayName, ex.Message);
return new List();
diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
index f4ea22f46..38df9876e 100644
--- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
@@ -9,11 +9,13 @@
using System;
using System.Collections.Generic;
using System.ComponentModel;
+using System.Globalization;
using System.Linq;
using System.Text.Json;
using System.Threading.Tasks;
using ModelContextProtocol.Server;
using Npgsql;
+using PerformanceMonitor.Alerting;
using PerformanceMonitor.Common;
#pragma warning disable CA1707 // MCP tools use snake_case naming convention
@@ -882,7 +884,7 @@ internal static string RenderServerList(
}, McpHelpers.JsonOptions);
}
- [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that.")]
+ [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server, instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process started - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
public static async Task GetCollectionHealth(
NpgsqlDataSource postgres,
[Description("Server name or display name.")] string? server_name = null)
@@ -1065,9 +1067,48 @@ never sustains. */
};
var peakCycleNote = SweepPressureClassifier.FormatPeakCycleNote(pressure);
+ /* #3013: the alerting layer's own store reads, which appear on no other health surface. A
+ condition check that cannot read the store logs one line and skips — correctly, since firing
+ on absent evidence fabricates an alert — but the skip is not a collector run, so it writes no
+ collection_log row and every field above this line stays green while the alert pass goes
+ blind one condition at a time. The key is derived the way THIS SKU's alert pass derives it
+ (invariant), so the read and the write land in the same bucket; AlertReadFailureSurfaceTests
+ pins that agreement from source rather than trusting it. */
+ var alertReads = AlertReadFailureCounter.Shared.ReadFor(
+ resolved.ServerId.ToString(CultureInfo.InvariantCulture));
+ var alertReadFinding = AlertReadFailureCounter.FormatFinding(alertReads);
+
return JsonSerializer.Serialize(new
{
server = resolved.ServerName,
+ /* #3013: a BLOCK rather than flat fields, unlike #3017's row-level output figures. There the
+ denominator (total_runs) was already on the row, so nesting the numerator away from it
+ would have split a ratio; here neither number exists on the response yet, so the block is
+ what keeps them together. Deliberately not a band and not a status input: any threshold
+ over it would have to guess how many blind reads make alerting unhealthy, and a wrong
+ guess on THIS surface fails in the direction #3013 is about. */
+ alert_read_health = new
+ {
+ /* Both scopes, because the two answer different questions and neither substitutes. The
+ per-server number is the actionable unit and matches this tool's scope; the instance
+ number is the only home the FLEET-scoped store self-alerts have — disk pressure,
+ compression-job health, store-job cadence, retention holds belong to no server, so a
+ per-server-only figure would have left them exactly as invisible as #3013 found the
+ whole class. */
+ server_read_failures = alertReads.ServerReadFailures,
+ server_alert_passes = alertReads.ServerAlertPasses,
+ instance_read_failures = alertReads.InstanceReadFailures,
+ /* The currency term, and the reason a count alone would be misread: 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. Exactly last_error's #2966 lesson. */
+ last_failure_at = alertReads.LastFailureAtUtc,
+ last_failure_read = alertReads.LastFailureRead,
+ /* The floor under the zero. A restart resets these counts, so counting_since is what
+ says whether a zero covers weeks or ninety seconds. */
+ counting_since = alertReads.CountingSinceUtc,
+ finding = alertReadFinding,
+ note = AlertReadFailureCounter.WindowNote
+ },
sweep_pressure = new
{
busy_ms_per_minute = Math.Round(pressure.BusyMsPerMinute, 0),
diff --git a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/server-tabs.js b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/server-tabs.js
index 6ea743119..669468768 100644
--- a/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/server-tabs.js
+++ b/Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/server-tabs.js
@@ -1298,6 +1298,12 @@ export const SERVER_TABS = [
heaviest query three times to open it. */
...fanout("get_collection_health", { server }, [
{ title: "Sweep Pressure", subtitle: "trailing 7 days", viz: "stat", stats: SWEEP_STATS },
+ /* #3013: the alerting layer's own store reads, which appear on no other health surface. Its own
+ panel rather than a tile on Sweep Pressure specifically because of the SUBTITLE: these figures are
+ in-memory counts since the service started, and inheriting "trailing 7 days" would have made the
+ panel assert a window it did not measure. Shared const so the SQL Server and PostgreSQL tabs cannot
+ drift apart on it. */
+ ALERT_READ_PANEL,
{
title: "Collectors",
subtitle: "trailing 7 days",
@@ -1438,6 +1444,12 @@ export const POSTGRES_TABS = [
three panels, for the reason fanout exists. */
...fanout("get_collection_health", { server }, [
{ title: "Sweep Pressure", subtitle: "trailing 7 days", viz: "stat", stats: SWEEP_STATS },
+ /* #3013: the alerting layer's own store reads, which appear on no other health surface. Its own
+ panel rather than a tile on Sweep Pressure specifically because of the SUBTITLE: these figures are
+ in-memory counts since the service started, and inheriting "trailing 7 days" would have made the
+ panel assert a window it did not measure. Shared const so the SQL Server and PostgreSQL tabs cannot
+ drift apart on it. */
+ ALERT_READ_PANEL,
{
title: "Collectors",
subtitle: "trailing 7 days",
@@ -2181,6 +2193,31 @@ const SESSION_STATS = [
{ key: "collection_time", label: "Collected", format: "reltime", small: true },
];
+/* #3013: the alerting subsystem's own swallowed store reads. Its own panel object (not just a stats array)
+ because the WINDOW is the point: every other panel on this tab is the trailing seven days, and this one is
+ an in-memory count since the service process started, which a restart takes to zero. The subtitle says so,
+ because a reader who assumed otherwise would read a zero as seven quiet days.
+
+ Deliberately carries no severity hint and feeds no band: a threshold here would have to guess how many
+ blind reads make alerting unhealthy, and on this surface a wrong guess fails by saying nothing is wrong.
+ "Newest" beside the counts for the same reason the Last Error column has a timestamp - the count never
+ ages out of a window, so a nonzero value with an old stamp is a healed episode. */
+const ALERT_READ_STATS = [
+ { key: "alert_read_health.server_read_failures", label: "Blind reads (server)", format: "int" },
+ { key: "alert_read_health.server_alert_passes", label: "Alert passes", format: "int" },
+ { key: "alert_read_health.instance_read_failures", label: "Blind reads (service)", format: "int" },
+ { key: "alert_read_health.last_failure_read", label: "Which read", format: "text", small: true },
+ { key: "alert_read_health.last_failure_at", label: "Newest", format: "reltime", small: true },
+ { key: "alert_read_health.counting_since", label: "Counting since", format: "reltime", small: true },
+];
+
+const ALERT_READ_PANEL = {
+ title: "Alerting Reads",
+ subtitle: "since this service started \u2014 NOT the trailing 7 days",
+ viz: "stat",
+ stats: ALERT_READ_STATS,
+};
+
const SWEEP_STATS = [
{ key: "sweep_pressure.verdict", label: "Verdict", format: "text", small: true },
{ key: "sweep_pressure.busy_percent", label: "Sweep busy %", format: "num1" },
diff --git a/Lite.Tests/AlertReadFailureSurfaceTests.cs b/Lite.Tests/AlertReadFailureSurfaceTests.cs
new file mode 100644
index 000000000..192a7bfc6
--- /dev/null
+++ b/Lite.Tests/AlertReadFailureSurfaceTests.cs
@@ -0,0 +1,275 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Reflection;
+using System.Runtime.CompilerServices;
+using System.Text.RegularExpressions;
+using PerformanceMonitor.Alerting;
+using Xunit;
+
+namespace Lite.Tests;
+
+///
+/// #3013's Lite half: get_collection_health exists on BOTH SKUs, so a block added to one is this
+/// repo's recurring parity failure (#3006 needed a test per SKU; #3017 found a fourth surface in the web
+/// dashboard). These pins are written from Lite's side and are deliberately NOT a copy of the Darling
+/// ones — they enumerate the SKU-paired surfaces and assert every one carries the block, rather than
+/// confirming the phrases this change just wrote appear where it wrote them.
+///
+/// What is NOT claimed. #3013's mechanism is store latency crossing the alert pass's
+/// Postgres command deadline, and Lite's alert reads hit a local DuckDB store, so that mechanism does not
+/// transfer. What transfers is the SURFACE gap: a swallowed alert read on Lite also reached no health read.
+/// The shared engine is where the counting happens, so Lite gets the same instrument for free and pays
+/// nothing for the parts of #3013 that are Darling's.
+///
+public sealed class AlertReadFailureSurfaceTests
+{
+ ///
+ /// Every file in the tree that DEFINES a get_collection_health MCP tool. Discovered rather than
+ /// listed: a positive check over the two files this change touched would confirm its own work and see
+ /// nothing else, which is how two older sites survived a parity sweep earlier in this backlog.
+ ///
+ private static IReadOnlyList CollectionHealthToolFiles()
+ {
+ var root = RepoRoot();
+
+ var found = Directory
+ .EnumerateFiles(root, "*.cs", SearchOption.AllDirectories)
+ .Where(p => !p.Contains($"{Path.DirectorySeparatorChar}obj{Path.DirectorySeparatorChar}", StringComparison.Ordinal)
+ && !p.Contains($"{Path.DirectorySeparatorChar}bin{Path.DirectorySeparatorChar}", StringComparison.Ordinal)
+ && !p.Contains($"{Path.DirectorySeparatorChar}deprecated{Path.DirectorySeparatorChar}", StringComparison.Ordinal)
+ && !p.Contains($"{Path.DirectorySeparatorChar}.git{Path.DirectorySeparatorChar}", StringComparison.Ordinal))
+ .Where(p => File.ReadAllText(p).Contains(
+ "[McpServerTool(Name = \"get_collection_health\")", StringComparison.Ordinal))
+ .OrderBy(p => p, StringComparer.Ordinal)
+ .ToList();
+
+ /* The control for the sweep above, through the identical enumeration: it must find MORE than one
+ file, or the whole-tree walk is silently reading nothing and every assertion built on it is
+ vacuous. Two is today's answer and the count is asserted at the call site; this is the floor. */
+ Assert.True(
+ found.Count >= 2,
+ $"the whole-tree walk for get_collection_health tool definitions found {found.Count} file(s) "
+ + "under " + root + " — a walk that reaches one file or none cannot make a parity claim");
+
+ return found;
+ }
+
+ [Fact]
+ public void EverySkusCollectionHealthTool_CarriesTheAlertReadBlock()
+ {
+ var files = CollectionHealthToolFiles();
+
+ Assert.Equal(2, files.Count);
+ Assert.Contains(files, f => f.EndsWith("McpHealthTools.cs", StringComparison.Ordinal));
+ Assert.Contains(files, f => f.EndsWith("DarlingMcpDataTools.cs", StringComparison.Ordinal));
+
+ foreach (var file in files)
+ {
+ var text = File.ReadAllText(file);
+
+ Assert.Contains("alert_read_health = new", text, StringComparison.Ordinal);
+ Assert.Contains("AlertReadFailureCounter.Shared.ReadFor(", text, StringComparison.Ordinal);
+ Assert.Contains("AlertReadFailureCounter.FormatFinding(", text, StringComparison.Ordinal);
+ Assert.Contains("note = AlertReadFailureCounter.WindowNote", text, StringComparison.Ordinal);
+ }
+ }
+
+ [Fact]
+ public void BothSkusBlocks_CarryTheIdenticalFieldSet_AndEveryFieldOfTheReading()
+ {
+ /* The parity claim proper. Compared as SETS of field names extracted from each SKU's own
+ initializer, so a field added to one and forgotten on the other fails here — and cross-checked
+ against the Reading record by REFLECTION, so a field added to the record and rendered by neither
+ SKU also fails. Two directions, because a payload nobody renders and a payload one SKU renders
+ are different defects with the same cause. */
+ var fieldsBySku = CollectionHealthToolFiles()
+ .ToDictionary(
+ f => Path.GetFileName(f),
+ f => AlertReadFieldNames(File.ReadAllText(f)),
+ StringComparer.Ordinal);
+
+ var sets = fieldsBySku.Values.ToList();
+ Assert.Equal(2, sets.Count);
+ Assert.Equal(sets[0], sets[1]);
+
+ var rendered = sets[0];
+
+ /* Reflected off the record so a seventh member cannot be added without a surface for it. */
+ var readingMembers = typeof(AlertReadFailureCounter.Reading)
+ .GetProperties(BindingFlags.Public | BindingFlags.Instance)
+ .Select(p => p.Name)
+ .Where(n => n != "EqualityContract")
+ .ToList();
+
+ Assert.Equal(6, readingMembers.Count);
+
+ /* Six from the record plus the two composed values. */
+ Assert.Equal(8, rendered.Count);
+ Assert.Equal(
+ new[]
+ {
+ "counting_since", "finding", "instance_read_failures", "last_failure_at",
+ "last_failure_read", "note", "server_alert_passes", "server_read_failures",
+ },
+ rendered.OrderBy(f => f, StringComparer.Ordinal).ToArray());
+ }
+
+ [Fact]
+ public void TheLiteSurface_DerivesTheSameServerKeyAsTheLiteAlertPass()
+ {
+ /* The silent-zero hazard, Lite's spelling of it. Lite's alert pass keys on
+ summary.ServerId.ToString() with no explicit culture, so the reader must render the key the SAME
+ way or it looks up a bucket nothing ever wrote and reports a confident zero. Same process, same
+ culture, so the two agree by construction — but only while they stay the same expression, which
+ is what this pins. Darling's pair uses InvariantCulture on both sides and is pinned from its own
+ side; neither SKU's spelling is imposed on the other, because changing Lite's alert key would
+ re-key its suppression, badge and watermark state as well. */
+ var tool = ReadSource(Path.Combine("Lite", "Mcp", "McpHealthTools.cs"));
+ var pass = ReadSource(Path.Combine("Lite", "MainWindow.AlertEngine.cs"));
+
+ Assert.Contains(
+ "AlertReadFailureCounter.Shared.ReadFor(resolved.ServerId.ToString())",
+ tool,
+ StringComparison.Ordinal);
+ Assert.Contains("var key = summary.ServerId.ToString();", pass, StringComparison.Ordinal);
+
+ /* The control: the same Contains form finds a deliberately wrong spelling nowhere, so its silence
+ above is an absence rather than a matcher that never matches. */
+ Assert.DoesNotContain(
+ "ReadFor(resolved.ServerId.ToString(CultureInfo.InvariantCulture))",
+ tool,
+ StringComparison.Ordinal);
+ }
+
+ [Fact]
+ public void TheCounter_IsWiredIntoLitesEngine_AtConstruction()
+ {
+ /* The wiring, which no behavioural test on this SKU can reach: the engine takes the counter as an
+ optional constructor argument defaulting to null, so an unwired Lite would compile, run, and
+ report a permanent zero. Exactly the #1648 middleware-ordering shape — a WIRING omission that a
+ pure logic pin passes straight over. */
+ var wiring = ReadSource(Path.Combine("Lite", "MainWindow.xaml.cs"));
+
+ Assert.Contains("readFailures: AlertReadFailureCounter.Shared);", wiring, StringComparison.Ordinal);
+
+ /* And the alias rather than a namespace import, because importing PerformanceMonitor.Alerting into
+ this file collides with the app's own CpuAlertMode — the reason the AlertEngine reference here is
+ an alias in the first place. */
+ Assert.Contains(
+ "using AlertReadFailureCounter = PerformanceMonitor.Alerting.AlertReadFailureCounter;",
+ wiring,
+ StringComparison.Ordinal);
+ }
+
+ [Fact]
+ public void BothSkusToolDescriptions_StayByteIdentical()
+ {
+ /* One tool, one contract: the two SKUs' descriptions of get_collection_health are byte-identical on
+ origin/dev and must stay so, or a client learns different things about the same tool depending on
+ which SKU answered. This change appended to both; the pin is that it appended the SAME bytes. */
+ var descriptions = CollectionHealthToolFiles()
+ .Select(f => ToolDescription(File.ReadAllText(f)))
+ .ToList();
+
+ Assert.Equal(2, descriptions.Count);
+ Assert.Equal(descriptions[0], descriptions[1]);
+
+ /* And that the appended paragraph is actually in there, so the equality above is not two copies of
+ an unchanged string agreeing with each other. */
+ Assert.Contains("alert_read_health", descriptions[0], StringComparison.Ordinal);
+ Assert.Contains("counting_since", descriptions[0], StringComparison.Ordinal);
+ Assert.Contains("failed to DELIVER", descriptions[0], StringComparison.Ordinal);
+ }
+
+ /* ---------------- helpers ---------------- */
+
+ /// The field names inside a tool's alert_read_health = new { … } initializer.
+ private static SortedSet AlertReadFieldNames(string source)
+ {
+ var at = source.IndexOf("alert_read_health = new", StringComparison.Ordinal);
+ Assert.True(at > 0, "a get_collection_health tool no longer builds an alert_read_health block");
+
+ var open = source.IndexOf('{', at);
+ Assert.True(open > 0, "alert_read_health has no initializer");
+
+ var depth = 0;
+ var end = -1;
+ for (var i = open; i < source.Length; i++)
+ {
+ if (source[i] == '{')
+ {
+ depth++;
+ }
+ else if (source[i] == '}')
+ {
+ depth--;
+ if (depth == 0)
+ {
+ end = i;
+ break;
+ }
+ }
+ }
+
+ Assert.True(end > open, "alert_read_health's initializer never closes");
+
+ var body = source[open..end];
+
+ /* Assignments only, and only at the initializer's own level — the block contains explanatory
+ comments with '=' in prose, so the pattern requires an identifier at the start of a line. */
+ var names = new SortedSet(StringComparer.Ordinal);
+ foreach (Match m in Regex.Matches(body, @"^\s*([a-z_][a-z0-9_]*)\s*=\s*[^=]", RegexOptions.Multiline))
+ {
+ names.Add(m.Groups[1].Value);
+ }
+
+ Assert.True(names.Count > 0, "no fields were extracted from an alert_read_health block");
+
+ return names;
+ }
+
+ private static string ToolDescription(string source)
+ {
+ var m = Regex.Match(
+ source,
+ @"\[McpServerTool\(Name = ""get_collection_health""\), Description\(""(.*?)""\)\]",
+ RegexOptions.Singleline);
+
+ Assert.True(m.Success, "a get_collection_health tool has no Description attribute in the expected shape");
+
+ return m.Groups[1].Value;
+ }
+
+ private static string ReadSource(string relative)
+ {
+ var path = Path.Combine(RepoRoot(), relative);
+
+ Assert.True(File.Exists(path), $"#3013 scan target not found: {path}");
+
+ return File.ReadAllText(path);
+ }
+
+ private static string RepoRoot([CallerFilePath] string thisFile = "")
+ {
+ var dir = Path.GetDirectoryName(thisFile)!;
+ while (dir is not null
+ && !File.Exists(Path.Combine(dir, "PerformanceMonitor.sln"))
+ && !Directory.Exists(Path.Combine(dir, ".git")))
+ {
+ dir = Path.GetDirectoryName(dir);
+ }
+
+ Assert.NotNull(dir);
+ return dir!;
+ }
+}
diff --git a/Lite/MainWindow.xaml.cs b/Lite/MainWindow.xaml.cs
index aea6325c8..1bb38362f 100644
--- a/Lite/MainWindow.xaml.cs
+++ b/Lite/MainWindow.xaml.cs
@@ -29,6 +29,7 @@
/* Type alias (not a namespace import) so PerformanceMonitor.Alerting's CpuAlertMode enum can never
collide with this app's own CpuAlertMode. */
using AlertEngine = PerformanceMonitor.Alerting.AlertEngine;
+using AlertReadFailureCounter = PerformanceMonitor.Alerting.AlertReadFailureCounter;
namespace PerformanceMonitorLite;
@@ -262,7 +263,14 @@ first fire-and-forget sweep. */
_muteRuleService.IsAlertMuted,
failedJobsFetcher: FetchFailedJobsForAlertAsync,
resolutionCallback: ShowAlertResolutionToastAsync,
- logger: new AppLoggerAdapter());
+ logger: new AppLoggerAdapter(),
+ /* #3013: the process counter every swallowed condition read is tallied on, which
+ get_collection_health's alert_read_health block reads back. Lite's alert reads hit the
+ local DuckDB store rather than a Postgres one, so the deadline mechanism #3013 measured
+ does not apply here — but the SURFACE gap does: a swallowed read reached no health read
+ on this SKU either. Passed explicitly rather than defaulted inside the engine so a test
+ constructs its own and cannot pollute this one. */
+ readFailures: AlertReadFailureCounter.Shared);
// Load mute rules from database
await _muteRuleService.LoadAsync();
diff --git a/Lite/Mcp/McpHealthTools.cs b/Lite/Mcp/McpHealthTools.cs
index 0a1a4561f..69d329fd0 100644
--- a/Lite/Mcp/McpHealthTools.cs
+++ b/Lite/Mcp/McpHealthTools.cs
@@ -2,6 +2,7 @@
using System.Text.Json;
using ModelContextProtocol.Server;
using PerformanceMonitorLite.Services;
+using AlertReadFailureCounter = PerformanceMonitor.Alerting.AlertReadFailureCounter;
using PerformanceMonitor.Common;
namespace PerformanceMonitorLite.Mcp;
@@ -202,7 +203,7 @@ and that difference is the most useful thing on this payload. */
}
}
- [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that.")]
+ [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server, instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process started - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
public static async Task GetCollectionHealth(
LocalDataService dataService,
ServerManager serverManager,
@@ -384,9 +385,51 @@ never sustains. */
};
var peakCycleNote = SweepPressureClassifier.FormatPeakCycleNote(pressure);
+ /* #3013: the alerting layer's own store reads, which appear on no other health surface. A
+ condition check that cannot read the store logs one line and skips - correctly, since firing
+ on absent evidence fabricates an alert - but the skip is not a collector run, so it writes no
+ collection_log row and every field above this line stays green while the alert pass goes
+ blind one condition at a time. The key is derived the way THIS SKU's alert pass derives it
+ (MainWindow.AlertEngine.cs's summary.ServerId.ToString()), so the read and the write land in
+ the same bucket; LiteAlertReadSurfaceTests pins that agreement from source rather than
+ trusting it. Lite's alert reads hit the local DuckDB store, so the deadline stratification
+ #3013 measured on a Postgres store does not apply here - but the SURFACE gap did, and this
+ block is a parity change, not a port of the mechanism. */
+ var alertReads = AlertReadFailureCounter.Shared.ReadFor(resolved.ServerId.ToString());
+ var alertReadFinding = AlertReadFailureCounter.FormatFinding(alertReads);
+
return JsonSerializer.Serialize(new
{
server = resolved.ServerName,
+ /* #3013: a BLOCK rather than flat fields, unlike #3017's row-level output figures. There the
+ denominator (total_runs) was already on the row, so nesting the numerator away from it
+ would have split a ratio; here neither number exists on the response yet, so the block is
+ what keeps them together. Deliberately not a band and not a status input: any threshold
+ over it would have to guess how many blind reads make alerting unhealthy, and a wrong
+ guess on THIS surface fails in the direction #3013 is about. */
+ alert_read_health = new
+ {
+ /* Both scopes, because the two answer different questions and neither substitutes. The
+ per-server number is the actionable unit and matches this tool's scope; the instance
+ number is the only home a failure belonging to no server has. On Lite every counted
+ read is per-server (the fleet-scoped store self-alerts are Darling's, and Lite has no
+ twin of them), so instance_read_failures here is the sum across this app's servers
+ rather than that sum plus a fleet bucket - the field means the same thing and is
+ reported for the same reason, and keeping it makes the two SKUs' payloads one shape. */
+ server_read_failures = alertReads.ServerReadFailures,
+ server_alert_passes = alertReads.ServerAlertPasses,
+ instance_read_failures = alertReads.InstanceReadFailures,
+ /* The currency term, and the reason a count alone would be misread: 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. Exactly last_error's #2966 lesson. */
+ last_failure_at = alertReads.LastFailureAtUtc,
+ last_failure_read = alertReads.LastFailureRead,
+ /* The floor under the zero. A restart resets these counts, so counting_since is what
+ says whether a zero covers weeks or ninety seconds. */
+ counting_since = alertReads.CountingSinceUtc,
+ finding = alertReadFinding,
+ note = AlertReadFailureCounter.WindowNote
+ },
sweep_pressure = new
{
busy_ms_per_minute = Math.Round(pressure.BusyMsPerMinute, 0),
diff --git a/PerformanceMonitor.Alerting/AlertEngine.cs b/PerformanceMonitor.Alerting/AlertEngine.cs
index 409400ffc..9d3c0128a 100644
--- a/PerformanceMonitor.Alerting/AlertEngine.cs
+++ b/PerformanceMonitor.Alerting/AlertEngine.cs
@@ -212,6 +212,14 @@ two plans failing on the same database are independent conditions that resolve i
///
/// Optional diagnostics logger.
/// Test seam for the cooldown clock; production leaves it null (UtcNow).
+ ///
+ /// Where a SWALLOWED condition read is counted (#3013). Every per-check catch below logs and skips —
+ /// correctly, because firing on absent evidence fabricates an alert and resolving on it fabricates a
+ /// recovery — but the skip reached no surface a person reads, so the alert pass could go blind one
+ /// condition at a time behind a green health read. Null leaves the counting off and changes nothing
+ /// else; production passes , and tests that want to
+ /// observe the counting pass their own instance rather than touching that one.
+ ///
public AlertEngine(
IAlertEngineSettings settings,
IAlertReadAdapter readAdapter,
@@ -221,7 +229,8 @@ public AlertEngine(
Func>>? failedJobsFetcher = null,
Func? resolutionCallback = null,
ILogger? logger = null,
- Func? utcNow = null)
+ Func? utcNow = null,
+ AlertReadFailureCounter? readFailures = null)
{
_settings = settings ?? throw new ArgumentNullException(nameof(settings));
_readAdapter = readAdapter ?? throw new ArgumentNullException(nameof(readAdapter));
@@ -232,8 +241,12 @@ public AlertEngine(
_resolutionCallback = resolutionCallback;
_logger = logger;
_utcNow = utcNow ?? (() => DateTime.UtcNow);
+ _readFailures = readFailures;
}
+ /// #3013: the swallowed-read counter, or null when nothing is counting.
+ private readonly AlertReadFailureCounter? _readFailures;
+
///
/// Runs one full alert sweep for one server — Lite's CheckPerformanceAlerts(summary).
/// Per-server serialized (see class remarks). Channel/store failures never escape (the
@@ -276,6 +289,11 @@ private async Task EvaluateCoreAsync(AlertServerSnapshot snaps
var alertCooldown = TimeSpan.FromMinutes(_settings.CooldownMinutes); /* :57 */
bool suppressed = snapshot.Suppressed; /* :60 (suppressPopups) */
+ /* #3013: the denominator for this server's swallowed-read count, recorded HERE rather than in
+ EvaluateServerAsync so the master-switch-off early return does not count a pass that never
+ looked at the store. */
+ _readFailures?.RecordPass(key);
+
await EnsureWatermarksSeededAsync(key, ct);
await CheckCpuAsync(snapshot, key, serverName, now, alertCooldown, suppressed, ct);
@@ -338,6 +356,7 @@ private async Task EnsureWatermarksSeededAsync(string key, CancellationToken ct)
catch (Exception ex)
{
_logger?.LogError("Failed to seed edge-trigger watermarks for {ServerKey}: {Message}", key, ex.Message);
+ _readFailures?.RecordReadFailure(key, "edge-trigger watermark seed");
}
_seededServerKeys[key] = true;
@@ -446,6 +465,7 @@ the merged count IS the DMV count. */
/* :129-132 shape — log and skip this check for the sweep (class remarks
adaptation (2)): never run the gate on a fabricated zero count. */
_logger?.LogError("Failed to check blocking for {Server}: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(key, "blocking");
return;
}
}
@@ -565,6 +585,9 @@ private async Task ObserveOccurrencesAsync(
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: an occurrence total is bookkeeping ABOUT an alert,
+ not the condition read the alert is judged on. The check still fires or resolves on its own
+ evidence when this fails, so alerting did not go blind — it lost a count. */
_logger?.LogWarning("Could not load incident occurrences for {Metric}: {Message}", metricName, ex.Message);
persisted = EmptyOccurrenceStates;
}
@@ -598,6 +621,8 @@ private async Task SaveOccurrencesAsync(
{
/* A dropped write costs accuracy on the next delivery's total — that fingerprint reads as new
and restarts, with a start time saying so — never a missed or duplicated alert. */
+ /* NOT counted by #3013's counter: a WRITE, and the counter is about reads the alert pass performs
+ and swallows. Logged at Warning for the same reason. */
_logger?.LogWarning("Could not persist incident occurrences for {Metric}: {Message}", metricName, ex.Message);
}
}
@@ -696,6 +721,7 @@ private async Task CheckBlockingWaitAsync(
/* Log and skip for the sweep — state untouched, so a transient store error neither
fires nor resolves (the same adaptation (2) shape as the count gate). */
_logger?.LogError("Failed to check blocking wait time for {Server}: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(key, "blocking wait time");
return;
}
}
@@ -784,6 +810,7 @@ private async Task CheckDeadlocksAsync(
{
/* :207-210 shape — log and skip (class remarks adaptation (2)). */
_logger?.LogError("Failed to check deadlocks for {Server}: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(key, "deadlocks");
return;
}
}
@@ -927,6 +954,7 @@ await NotifyResolutionAsync(new AlertResolution(
catch (Exception ex)
{
_logger?.LogError("Failed to check poison waits for {Server}: {Message}", serverName, ex.Message); /* :337 */
+ _readFailures?.RecordReadFailure(key, "poison waits");
}
}
@@ -1016,6 +1044,7 @@ await NotifyResolutionAsync(new AlertResolution(
catch (Exception ex)
{
_logger?.LogError("Failed to check long-running queries for {Server}: {Message}", serverName, ex.Message); /* :409 */
+ _readFailures?.RecordReadFailure(key, "long-running queries");
}
}
@@ -1077,6 +1106,7 @@ await NotifyResolutionAsync(new AlertResolution(
catch (Exception ex)
{
_logger?.LogError("Failed to check TempDB space for {Server}: {Message}", serverName, ex.Message); /* :471 */
+ _readFailures?.RecordReadFailure(key, "TempDB space");
}
}
@@ -1166,6 +1196,7 @@ await NotifyResolutionAsync(new AlertResolution(
catch (Exception ex)
{
_logger?.LogError("Failed to check volume free space for {Server}: {Message}", serverName, ex.Message); /* :553 */
+ _readFailures?.RecordReadFailure(key, "volume free space");
}
return conditionPresent;
@@ -1252,6 +1283,7 @@ await NotifyResolutionAsync(new AlertResolution(
catch (Exception ex)
{
_logger?.LogError("Failed to check PVS pressure for {Server}: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(key, "PVS pressure");
}
}
@@ -1350,6 +1382,7 @@ await NotifyResolutionAsync(new AlertResolution(
catch (Exception ex)
{
_logger?.LogError("Failed to check database file growth for {Server}: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(key, "database file growth");
}
}
@@ -1444,6 +1477,7 @@ await NotifyResolutionAsync(new AlertResolution(
catch (Exception ex)
{
_logger?.LogError("Failed to check anomalous jobs for {Server}: {Message}", serverName, ex.Message); /* :630 */
+ _readFailures?.RecordReadFailure(key, "anomalous jobs");
}
}
@@ -1533,6 +1567,7 @@ await FireAsync(new AlertOutcome(
catch (Exception ex)
{
_logger?.LogError("Failed to check failed jobs for {Server}: {Message}", serverName, ex.Message); /* :715 */
+ _readFailures?.RecordReadFailure(key, "failed jobs");
}
return conditionPresent;
@@ -1576,6 +1611,7 @@ private async Task CheckDatabaseStateAsync(
/* Log-and-skip, like the other collected reads: never resolve an active database on a
failed fetch (that would fabricate a recovery), and never fire on absent evidence. */
_logger?.LogError("Failed to check database state for {Server}: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(key, "database state");
return;
}
@@ -1787,6 +1823,7 @@ private async Task CheckForcePlanFailuresAsync(
/* Log-and-skip, like every other collected read: never resolve an active plan on a failed
fetch (that would fabricate a recovery), and never fire on absent evidence. */
_logger?.LogError("Failed to check forced-plan failures for {Server}: {Message}", serverName, ex.Message);
+ _readFailures?.RecordReadFailure(key, "forced-plan failures");
return;
}
@@ -1955,6 +1992,9 @@ private async Task NotifyResolutionAsync(AlertResolution resolution, Cancellatio
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter: this is the DELIVERY path, not a condition read.
+ A failed delivery is a different fact with a different remedy, and #3013 deliberately left
+ alerting on the alerting out of scope as its own decision. */
_logger?.LogError("Alert resolution callback failed for {Server} / {Metric}: {Message}",
resolution.ServerName, resolution.MetricName, ex.Message);
}
diff --git a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
new file mode 100644
index 000000000..f99c39241
--- /dev/null
+++ b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
@@ -0,0 +1,298 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System;
+using System.Collections.Concurrent;
+using System.Collections.Generic;
+using System.Globalization;
+using System.Linq;
+using System.Threading;
+
+namespace PerformanceMonitor.Alerting;
+
+///
+/// Counts the store reads the alerting layer performed, failed, and SWALLOWED (#3013).
+///
+/// The blind spot this closes. Every condition check in the alert pass wraps its read in
+/// log-and-skip: on a failure it writes one [ERROR line and returns, because firing on absent
+/// evidence would fabricate an alert and resolving on it would fabricate a recovery. That posture is
+/// correct and stays. What was missing is that a swallowed read reaches 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 was going blind one condition at a
+/// time. Only a grep of the service log found it.
+///
+/// Why it matters more than the raw count suggests. The alert pass runs on
+/// DarlingAlertReadAdapter.AlertPassCommandTimeoutSeconds while the collection sweep runs on
+/// budgets an order of magnitude longer. As store latency rises the SHORT-deadline consumers cross
+/// their limit first, so the failure ordering under store contention is: alerting first, collection
+/// last. During one measured episode of store-side lock contention the service log's [ERROR
+/// rate rose 41 → 61 per hour, every line an alerting-side store read, while collector failures in
+/// collection_log FELL over the same hours (23, 7, 5, 2). Two populations moving in opposite
+/// directions, and the rising one was the invisible one.
+///
+/// Deliberately in memory, and deliberately not persisted. The thing being counted 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. Every consumer of this count lives in the same
+/// process as the alert pass that produces it (both SKUs host their MCP surface in-process), so there
+/// is nothing to persist it for. The cost is stated rather than hidden: the count covers this process
+/// only, from , and a restart takes it to zero — see
+/// , which says so on the surface rather than leaving a reader to assume it
+/// shares the seven-day window the collector rows carry.
+///
+/// Not a band. This reports counts, a currency stamp and the name of the read that failed
+/// most recently. It feeds no verdict and no health status, for #3017's reason one level down: any
+/// threshold over it would have to guess how many failed reads make alerting "unhealthy", and a wrong
+/// guess on a surface like this one either cries wolf or — worse, and the failure mode #3013 is about —
+/// says nothing is wrong.
+///
+/// Keying. Per-server counts are held under the alert pass's own server key, verbatim and
+/// ordinal, so a reader must derive the key the same way its own SKU's alert pass does (Darling:
+/// serverId.ToString(CultureInfo.InvariantCulture); Lite: serverId.ToString()). Both are
+/// same-process reads of the same rendering, so they agree by construction — and
+/// AlertReadFailureSurfaceTests pins the agreement from source rather than trusting it. Failures
+/// belonging to no server (the fleet-scoped store self-alerts — disk pressure, compression-job health,
+/// store-job cadence, retention holds) are recorded with a null key: they land in the instance total and
+/// in no server's count, which is why the surface reports BOTH numbers. A per-server-only figure would
+/// have given those conditions no home at all, reproducing #3013's own defect one level down.
+///
+/// Thread-safety. Many alert passes run concurrently across servers. Counters are
+/// longs; the per-server map is a
+/// of boxed holders so an increment never replaces an entry. takes each number
+/// with a volatile read and makes no claim that the fields it returns are one atomic instant — a total
+/// and a per-server count sampled a microsecond apart is not a defect this surface can be misread on.
+///
+public sealed class AlertReadFailureCounter
+{
+ ///
+ /// The process's counter — the one the alert pass writes and the MCP surfaces read.
+ ///
+ /// A static well-known instance rather than a container registration because the value is
+ /// process-global by nature and the two readers (each SKU's get_collection_health) are
+ /// static tool methods in a DI container built separately from the one the alert pass is
+ /// constructed in. The WRITE side is still injected — every producer takes a nullable
+ /// and production passes this instance explicitly — so a
+ /// test constructs its own and cannot pollute this one.
+ ///
+ public static AlertReadFailureCounter Shared { get; } = new AlertReadFailureCounter();
+
+ private sealed class ServerCounts
+ {
+ public long ReadFailures;
+ public long Passes;
+ public long LastFailureTicks;
+ public string? LastFailureRead;
+ }
+
+ private readonly ConcurrentDictionary _byServer =
+ new ConcurrentDictionary(StringComparer.Ordinal);
+
+ /* Failures that belong to no server: the fleet-scoped store self-alerts. Held apart from the
+ per-server map rather than under a sentinel key, so no reader can accidentally resolve a server
+ named after the sentinel and be handed the fleet bucket. */
+ private readonly ServerCounts _fleet = new ServerCounts();
+
+ private long _instanceReadFailures;
+ private long _instanceLastFailureTicks;
+ private string? _instanceLastFailureRead;
+
+ private readonly Func _utcNow;
+
+ /// When this counter started counting — process start for .
+ public DateTime CountingSince { get; }
+
+ public AlertReadFailureCounter(Func? utcNow = null)
+ {
+ _utcNow = utcNow ?? (() => DateTime.UtcNow);
+ CountingSince = _utcNow();
+ }
+
+ ///
+ /// Records one alerting-side store read that failed and was swallowed.
+ ///
+ ///
+ /// The alert pass's server key, or null for a condition that belongs to no server (the fleet-scoped
+ /// store self-alerts). A null or blank key lands in the fleet bucket and the instance total.
+ ///
+ ///
+ /// A short, CONSTANT name for the read that failed — "deadlocks", "forced-plan failures",
+ /// "collection-health self-alert". It is the actionable half of the count: which condition went
+ /// blind, rather than merely that something did.
+ ///
+ /// Deliberately not the exception message. Npgsql renders both a deadline and an unreachable
+ /// backend as the same seven words, so the message adds no information the count does not already
+ /// carry — and an exception message can carry host and database names, which must not reach an MCP
+ /// response.
+ ///
+ public void RecordReadFailure(string? serverKey, string readName)
+ {
+ var name = string.IsNullOrWhiteSpace(readName) ? "unnamed read" : readName;
+ var nowTicks = _utcNow().Ticks;
+
+ var bucket = string.IsNullOrWhiteSpace(serverKey) ? _fleet : Bucket(serverKey!);
+ Interlocked.Increment(ref bucket.ReadFailures);
+ Interlocked.Exchange(ref bucket.LastFailureTicks, nowTicks);
+ bucket.LastFailureRead = name;
+
+ Interlocked.Increment(ref _instanceReadFailures);
+ Interlocked.Exchange(ref _instanceLastFailureTicks, nowTicks);
+ _instanceLastFailureRead = name;
+ }
+
+ ///
+ /// Records one alert evaluation pass for a server — the denominator the failure count is read
+ /// against. Deliberately a raw count and not a rate: 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, and each pass issues many reads, so no quotient of these two numbers names
+ /// anything. What the denominator is FOR is telling three failures over two hundred passes apart
+ /// from three over four.
+ ///
+ public void RecordPass(string? serverKey)
+ {
+ var bucket = string.IsNullOrWhiteSpace(serverKey) ? _fleet : Bucket(serverKey!);
+ Interlocked.Increment(ref bucket.Passes);
+ }
+
+ private ServerCounts Bucket(string serverKey) =>
+ _byServer.GetOrAdd(serverKey, _ => new ServerCounts());
+
+ ///
+ /// One server's alerting-read health plus the instance totals it sits inside.
+ ///
+ /// Swallowed alerting-side store reads for THIS server.
+ /// Alert evaluation passes for this server — the denominator.
+ ///
+ /// Swallowed alerting-side store reads across every server AND the fleet-scoped store self-alerts,
+ /// which belong to no server and would otherwise appear nowhere.
+ ///
+ ///
+ /// When the newest failure for this server happened, or null if it has none. The currency term: a
+ /// nonzero count with a stamp from days ago is a healed episode, and a count with no stamp beside it
+ /// is the mistake last_error already taught this surface (#2966).
+ ///
+ /// Which read failed most recently for this server.
+ /// When counting began — process start.
+ public sealed record Reading(
+ long ServerReadFailures,
+ long ServerAlertPasses,
+ long InstanceReadFailures,
+ DateTime? LastFailureAtUtc,
+ string? LastFailureRead,
+ DateTime CountingSinceUtc);
+
+ /// Reads one server's figures. An unseen key reads as zeroes, not as an absence.
+ public Reading ReadFor(string? serverKey)
+ {
+ var bucket = string.IsNullOrWhiteSpace(serverKey) ? null : Lookup(serverKey!);
+ var serverFailures = bucket is null ? 0L : Interlocked.Read(ref bucket.ReadFailures);
+ var serverPasses = bucket is null ? 0L : Interlocked.Read(ref bucket.Passes);
+ var lastTicks = bucket is null ? 0L : Interlocked.Read(ref bucket.LastFailureTicks);
+
+ return new Reading(
+ serverFailures,
+ serverPasses,
+ Interlocked.Read(ref _instanceReadFailures),
+ lastTicks == 0 ? null : new DateTime(lastTicks, DateTimeKind.Utc),
+ bucket?.LastFailureRead,
+ CountingSince);
+ }
+
+ private ServerCounts? Lookup(string serverKey) =>
+ _byServer.TryGetValue(serverKey, out var counts) ? counts : null;
+
+ /// The instance-wide figures, for a caller with no server in hand.
+ public (long ReadFailures, DateTime? LastFailureAtUtc, string? LastFailureRead) ReadInstance()
+ {
+ var lastTicks = Interlocked.Read(ref _instanceLastFailureTicks);
+ return (
+ Interlocked.Read(ref _instanceReadFailures),
+ lastTicks == 0 ? null : new DateTime(lastTicks, DateTimeKind.Utc),
+ _instanceLastFailureRead);
+ }
+
+ /// Every server key that has recorded a pass or a failure — for a fleet-level reader.
+ public IReadOnlyList ServerKeys() =>
+ _byServer.Keys.OrderBy(k => k, StringComparer.Ordinal).ToList();
+
+ ///
+ /// The sentence that names WHICH read went blind and when, or null when nothing has failed.
+ ///
+ /// Display text, exactly like #3017's output_finding: it states a fact and recommends
+ /// nothing. Composed here so the two SKUs' tools and the web panel cannot render it three ways.
+ ///
+ public static string? FormatFinding(Reading reading)
+ {
+ if (reading is null)
+ {
+ throw new ArgumentNullException(nameof(reading));
+ }
+
+ if (reading.ServerReadFailures == 0 && reading.InstanceReadFailures == 0)
+ {
+ return null;
+ }
+
+ if (reading.ServerReadFailures == 0)
+ {
+ return string.Format(
+ CultureInfo.InvariantCulture,
+ "No alerting-side store read has failed for this server, but {0} failed elsewhere in this "
+ + "service since {1:yyyy-MM-dd HH:mm}Z — on another server, or on a store self-alert that "
+ + "belongs to no server. This server's alerting is reading fine; the service's is not "
+ + "entirely.",
+ reading.InstanceReadFailures,
+ reading.CountingSinceUtc);
+ }
+
+ var stamp = reading.LastFailureAtUtc.HasValue
+ ? string.Format(CultureInfo.InvariantCulture, ", newest at {0:yyyy-MM-dd HH:mm:ss}Z", reading.LastFailureAtUtc.Value)
+ : string.Empty;
+
+ var which = string.IsNullOrWhiteSpace(reading.LastFailureRead)
+ ? string.Empty
+ : string.Format(CultureInfo.InvariantCulture, " The newest was the {0} read.", reading.LastFailureRead);
+
+ return string.Format(
+ CultureInfo.InvariantCulture,
+ "{0} alerting-side store read(s) for this server failed and were logged but reached no health "
+ + "surface{1}, over {2} alert pass(es) since {3:yyyy-MM-dd HH:mm}Z. Each one is a condition this "
+ + "server was not judged on for that pass — not a fired alert that was lost, and not a collector "
+ + "failure ({4} across this whole service).{5}",
+ reading.ServerReadFailures,
+ stamp,
+ reading.ServerAlertPasses,
+ reading.CountingSinceUtc,
+ reading.InstanceReadFailures,
+ which);
+ }
+
+ ///
+ /// The window these figures cover, and the window they do NOT.
+ ///
+ /// Said out loud because this block is the one part of get_collection_health that is not
+ /// on the response's seven-day window, and a reader who assumed it was would read a zero as "no
+ /// alerting failures in seven days" when a restart minutes ago is all it means. #3017's
+ /// output_note established the discipline; this is the same claim about a different window.
+ ///
+ public const string WindowNote =
+ "alert_read_health is the ONLY block on this response that is not measured over the trailing seven "
+ + "days. It is an in-memory count kept by the running service or app, from counting_since — which is "
+ + "when this process started — to now, and a restart takes it to zero. So a zero here means "
+ + "\"none since counting_since\" and NOT \"none in seven days\": check counting_since before reading "
+ + "the zero as reassurance, because a process that started a minute ago can only report on a minute. "
+ + "It is 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 report. It counts "
+ + "alerting-side store reads that failed and were swallowed by design (the alert pass logs and skips "
+ + "rather than firing or resolving on absent evidence), which is why they appear on no other health "
+ + "surface: they are not collector runs and write no collection_log row. It does NOT count fired "
+ + "alerts that failed to DELIVER, and it makes no claim about them — that is the alert-history read's "
+ + "question, not this one. instance_read_failures spans every server on this service plus the "
+ + "fleet-scoped store self-alerts (disk pressure, compression-job health, store-job cadence, "
+ + "retention holds), which belong to no server and so appear in no per-server count. "
+ + "server_alert_passes is a denominator for judging whether the failure count is large, not a rate: a "
+ + "pass issues many reads, and Darling runs two passes per sweep where Lite runs one.";
+}
From c60c270128a30d2af96154ea652b57c29539b991 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 15:33:49 -0400
Subject: [PATCH 02/11] Say what counting_since is precisely, and drop a
null-conditional that can never fire
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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.
---
.../DarlingWorker.cs | 19 +++++++++++--------
.../Mcp/DarlingMcpDataTools.cs | 2 +-
Lite/Mcp/McpHealthTools.cs | 2 +-
.../AlertReadFailureCounter.cs | 17 +++++++++++++++--
4 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
index 3b918893c..dae19edfa 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
@@ -3204,7 +3204,9 @@ PostgreSQL read. */
catch (Exception ex)
{
_logger.LogError("[{Server}] Alert sweep failed: {Message}", server.Config.DisplayName, ex.Message);
- _readFailures?.RecordReadFailure(runtime.ServerId.ToString(CultureInfo.InvariantCulture), "alert pass (latest-CPU read and the shared engine sweep)");
+ _readFailures.RecordReadFailure(
+ runtime.ServerId.ToString(CultureInfo.InvariantCulture),
+ "alert pass (latest-CPU read and the shared engine sweep)");
}
}
@@ -3320,7 +3322,7 @@ different subject does not. */
{
_logger.LogError("[{Server}] PostgreSQL alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
- _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL outage-predictor reads");
+ _readFailures.RecordReadFailure(snapshot.ServerKey, "PostgreSQL outage-predictor reads");
}
/* #2711/#2719: Deadlocks, Blocking, Long-Running Query, Poison Wait and High CPU, each
@@ -3422,7 +3424,7 @@ reading is null
{
_logger.LogError("[{Server}] PostgreSQL CPU alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
- _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL CPU alert read");
+ _readFailures.RecordReadFailure(snapshot.ServerKey, "PostgreSQL CPU alert read");
}
}
@@ -3536,7 +3538,7 @@ await NotifyPgResolutionAsync(key, snapshot.ServerName, metricName, "Deadlocks C
{
_logger.LogError("[{Server}] PostgreSQL deadlock alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
- _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL deadlock alert read");
+ _readFailures.RecordReadFailure(snapshot.ServerKey, "PostgreSQL deadlock alert read");
}
}
@@ -3647,7 +3649,7 @@ await NotifyPgResolutionAsync(key, snapshot.ServerName, metricName, "Blocking Cl
{
_logger.LogError("[{Server}] PostgreSQL blocking alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
- _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL blocking alert read");
+ _readFailures.RecordReadFailure(snapshot.ServerKey, "PostgreSQL blocking alert read");
}
}
@@ -3764,7 +3766,7 @@ await NotifyPgResolutionAsync(key, snapshot.ServerName, metricName, "Long-Runnin
{
_logger.LogError("[{Server}] PostgreSQL long-running-query alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
- _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL long-running-query alert read");
+ _readFailures.RecordReadFailure(snapshot.ServerKey, "PostgreSQL long-running-query alert read");
}
}
@@ -3967,7 +3969,7 @@ await NotifyPgResolutionAsync(serverKey, snapshot.ServerName, metricName, "Poiso
{
_logger.LogError("[{Server}] PostgreSQL poison wait alert evaluation failed: {Message}",
runtime.Config.DisplayName, ex.Message);
- _readFailures?.RecordReadFailure(snapshot.ServerKey, "PostgreSQL poison wait alert read");
+ _readFailures.RecordReadFailure(snapshot.ServerKey, "PostgreSQL poison wait alert read");
}
}
@@ -4339,7 +4341,8 @@ Judged on the CONSEQUENCE (held AND the tier past its own horizon), never on the
catch (Exception ex)
{
_logger.LogError("Compression-job health check failed: {Message}", ex.Message);
- _readFailures?.RecordReadFailure(null, "store background-job health reads (compression, job cadence, retention holds)");
+ _readFailures.RecordReadFailure(
+ null, "store background-job health reads (compression, job cadence, retention holds)");
}
}
diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
index 38df9876e..b9dc577c5 100644
--- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
@@ -884,7 +884,7 @@ internal static string RenderServerList(
}, McpHelpers.JsonOptions);
}
- [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server, instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process started - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
+ [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server, instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
public static async Task GetCollectionHealth(
NpgsqlDataSource postgres,
[Description("Server name or display name.")] string? server_name = null)
diff --git a/Lite/Mcp/McpHealthTools.cs b/Lite/Mcp/McpHealthTools.cs
index 69d329fd0..719740c6b 100644
--- a/Lite/Mcp/McpHealthTools.cs
+++ b/Lite/Mcp/McpHealthTools.cs
@@ -203,7 +203,7 @@ and that difference is the most useful thing on this payload. */
}
}
- [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server, instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process started - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
+ [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server, instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
public static async Task GetCollectionHealth(
LocalDataService dataService,
ServerManager serverManager,
diff --git a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
index f99c39241..979f62b6e 100644
--- a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
+++ b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
@@ -151,6 +151,9 @@ public void RecordReadFailure(string? serverKey, string readName)
/// anything. What the denominator is FOR is telling three failures over two hundred passes apart
/// from three over four.
///
+ /// Nullable for symmetry with , but no caller passes null
+ /// today: both SKUs' passes are per-server, and the fleet-scoped store self-alerts are polled on
+ /// their own cadences rather than as a pass over a server.
public void RecordPass(string? serverKey)
{
var bucket = string.IsNullOrWhiteSpace(serverKey) ? _fleet : Bucket(serverKey!);
@@ -184,7 +187,16 @@ public sealed record Reading(
string? LastFailureRead,
DateTime CountingSinceUtc);
- /// Reads one server's figures. An unseen key reads as zeroes, not as an absence.
+ ///
+ /// Reads one server's figures. An unseen key reads as zeroes, not as an absence — the surface
+ /// serializes this straight into JSON, and a null-shaped reading for a server that simply has not
+ /// failed would render as a block of nulls for a reader to interpret.
+ ///
+ /// A null or blank key is deliberately NOT a route to the fleet bucket, even though
+ /// writes there for one: this is the PER-SERVER read, and a caller
+ /// with no server in hand wants . The fleet bucket still reaches every
+ /// reader through InstanceReadFailures, which is the field that exists for it.
+ ///
public Reading ReadFor(string? serverKey)
{
var bucket = string.IsNullOrWhiteSpace(serverKey) ? null : Lookup(serverKey!);
@@ -281,7 +293,8 @@ public IReadOnlyList ServerKeys() =>
public const string WindowNote =
"alert_read_health is the ONLY block on this response that is not measured over the trailing seven "
+ "days. It is an in-memory count kept by the running service or app, from counting_since — which is "
- + "when this process started — to now, and a restart takes it to zero. So a zero here means "
+ + "when this process began counting, early in its own startup — to now, and a restart takes it to "
+ + "zero. So a zero here means "
+ "\"none since counting_since\" and NOT \"none in seven days\": check counting_since before reading "
+ "the zero as reassurance, because a process that started a minute ago can only report on a minute. "
+ "It is deliberately not persisted: what it counts is a failure to READ the store, so a counter that "
From 8b86b62b1500b3c0b2dfccbb51f025f1fd9c3e58 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 15:37:13 -0400
Subject: [PATCH 03/11] 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.
---
.../Mcp/DarlingMcpDataTools.cs | 8 ++++++--
Lite/Mcp/McpHealthTools.cs | 8 ++++++--
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
index b9dc577c5..ef3406a96 100644
--- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
@@ -1101,11 +1101,15 @@ whole class. */
/* The currency term, and the reason a count alone would be misread: 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. Exactly last_error's #2966 lesson. */
- last_failure_at = alertReads.LastFailureAtUtc,
+ /* Round-trip "o", matching last_success / last_error_at / last_denied_at on the
+ collector rows of this same response. A raw DateTime would serialize to an ISO
+ string too, but with trailing zeros trimmed, so two timestamps on one payload
+ would carry different precision guarantees for no reason. */
+ last_failure_at = alertReads.LastFailureAtUtc?.ToString("o"),
last_failure_read = alertReads.LastFailureRead,
/* The floor under the zero. A restart resets these counts, so counting_since is what
says whether a zero covers weeks or ninety seconds. */
- counting_since = alertReads.CountingSinceUtc,
+ counting_since = alertReads.CountingSinceUtc.ToString("o"),
finding = alertReadFinding,
note = AlertReadFailureCounter.WindowNote
},
diff --git a/Lite/Mcp/McpHealthTools.cs b/Lite/Mcp/McpHealthTools.cs
index 719740c6b..8f4fa7390 100644
--- a/Lite/Mcp/McpHealthTools.cs
+++ b/Lite/Mcp/McpHealthTools.cs
@@ -422,11 +422,15 @@ rather than that sum plus a fleet bucket - the field means the same thing and is
/* The currency term, and the reason a count alone would be misread: 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. Exactly last_error's #2966 lesson. */
- last_failure_at = alertReads.LastFailureAtUtc,
+ /* Round-trip "o", matching last_success / last_error_at / last_denied_at on the
+ collector rows of this same response. A raw DateTime would serialize to an ISO
+ string too, but with trailing zeros trimmed, so two timestamps on one payload
+ would carry different precision guarantees for no reason. */
+ last_failure_at = alertReads.LastFailureAtUtc?.ToString("o"),
last_failure_read = alertReads.LastFailureRead,
/* The floor under the zero. A restart resets these counts, so counting_since is what
says whether a zero covers weeks or ninety seconds. */
- counting_since = alertReads.CountingSinceUtc,
+ counting_since = alertReads.CountingSinceUtc.ToString("o"),
finding = alertReadFinding,
note = AlertReadFailureCounter.WindowNote
},
From 651f5b066342aa2554d118a303b126849363bc75 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 15:39:21 -0400
Subject: [PATCH 04/11] 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.
---
.../Mcp/DarlingMcpDataTools.cs | 11 ++++++-----
Lite/Mcp/McpHealthTools.cs | 13 +++++++------
.../AlertReadFailureCounter.cs | 2 +-
3 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
index ef3406a96..28c937bd8 100644
--- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
@@ -1100,11 +1100,12 @@ whole class. */
instance_read_failures = alertReads.InstanceReadFailures,
/* The currency term, and the reason a count alone would be misread: 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. Exactly last_error's #2966 lesson. */
- /* Round-trip "o", matching last_success / last_error_at / last_denied_at on the
- collector rows of this same response. A raw DateTime would serialize to an ISO
- string too, but with trailing zeros trimmed, so two timestamps on one payload
- would carry different precision guarantees for no reason. */
+ one still in progress read identically. Exactly last_error's #3010 lesson.
+
+ Round-trip "o", matching last_success / last_error_at / last_denied_at on the
+ collector rows of this same response. A raw DateTime would serialize to an ISO string
+ too, but with trailing zeros trimmed, so two timestamps on one payload would carry
+ different precision guarantees for no reason. */
last_failure_at = alertReads.LastFailureAtUtc?.ToString("o"),
last_failure_read = alertReads.LastFailureRead,
/* The floor under the zero. A restart resets these counts, so counting_since is what
diff --git a/Lite/Mcp/McpHealthTools.cs b/Lite/Mcp/McpHealthTools.cs
index 8f4fa7390..44510b1e9 100644
--- a/Lite/Mcp/McpHealthTools.cs
+++ b/Lite/Mcp/McpHealthTools.cs
@@ -391,7 +391,7 @@ never sustains. */
collection_log row and every field above this line stays green while the alert pass goes
blind one condition at a time. The key is derived the way THIS SKU's alert pass derives it
(MainWindow.AlertEngine.cs's summary.ServerId.ToString()), so the read and the write land in
- the same bucket; LiteAlertReadSurfaceTests pins that agreement from source rather than
+ the same bucket; Lite.Tests' AlertReadFailureSurfaceTests pins that agreement from source rather than
trusting it. Lite's alert reads hit the local DuckDB store, so the deadline stratification
#3013 measured on a Postgres store does not apply here - but the SURFACE gap did, and this
block is a parity change, not a port of the mechanism. */
@@ -421,11 +421,12 @@ rather than that sum plus a fleet bucket - the field means the same thing and is
instance_read_failures = alertReads.InstanceReadFailures,
/* The currency term, and the reason a count alone would be misread: 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. Exactly last_error's #2966 lesson. */
- /* Round-trip "o", matching last_success / last_error_at / last_denied_at on the
- collector rows of this same response. A raw DateTime would serialize to an ISO
- string too, but with trailing zeros trimmed, so two timestamps on one payload
- would carry different precision guarantees for no reason. */
+ one still in progress read identically. Exactly last_error's #3010 lesson.
+
+ Round-trip "o", matching last_success / last_error_at / last_denied_at on the
+ collector rows of this same response. A raw DateTime would serialize to an ISO string
+ too, but with trailing zeros trimmed, so two timestamps on one payload would carry
+ different precision guarantees for no reason. */
last_failure_at = alertReads.LastFailureAtUtc?.ToString("o"),
last_failure_read = alertReads.LastFailureRead,
/* The floor under the zero. A restart resets these counts, so counting_since is what
diff --git a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
index 979f62b6e..54ff02581 100644
--- a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
+++ b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
@@ -175,7 +175,7 @@ private ServerCounts Bucket(string serverKey) =>
///
/// When the newest failure for this server happened, or null if it has none. The currency term: a
/// nonzero count with a stamp from days ago is a healed episode, and a count with no stamp beside it
- /// is the mistake last_error already taught this surface (#2966).
+ /// is the mistake last_error already taught this surface (#3010).
///
/// Which read failed most recently for this server.
/// When counting began — process start.
From c63d681e636b26b24c243ea37c8d8da7d140d4e5 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 15:45:01 -0400
Subject: [PATCH 05/11] 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.
---
PerformanceMonitor.Alerting/AlertReadFailureCounter.cs | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
index 54ff02581..1ebc93eee 100644
--- a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
+++ b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
@@ -102,7 +102,9 @@ named after the sentinel and be handed the fleet bucket. */
private readonly Func _utcNow;
- /// When this counter started counting — process start for .
+ /// When this counter first counted. For that is the first touch of the
+ /// static — early in the host's startup, not the process's first instruction, which is why the
+ /// surface reports the value rather than describing it.
public DateTime CountingSince { get; }
public AlertReadFailureCounter(Func? utcNow = null)
@@ -178,7 +180,7 @@ private ServerCounts Bucket(string serverKey) =>
/// is the mistake last_error already taught this surface (#3010).
///
/// Which read failed most recently for this server.
- /// When counting began — process start.
+ /// When counting began — see .
public sealed record Reading(
long ServerReadFailures,
long ServerAlertPasses,
@@ -294,8 +296,8 @@ public IReadOnlyList ServerKeys() =>
"alert_read_health is the ONLY block on this response that is not measured over the trailing seven "
+ "days. It is an in-memory count kept by the running service or app, from counting_since — which is "
+ "when this process began counting, early in its own startup — to now, and a restart takes it to "
- + "zero. So a zero here means "
- + "\"none since counting_since\" and NOT \"none in seven days\": check counting_since before reading "
+ + "zero. So a zero here means \"none since counting_since\" and NOT \"none in seven days\": check "
+ + "counting_since before reading "
+ "the zero as reassurance, because a process that started a minute ago can only report on a minute. "
+ "It is 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 report. It counts "
From c1a68d1b26accf891bc67da74acde3fc94822312 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 15:56:44 -0400
Subject: [PATCH 06/11] 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.
---
.../AlertReadFailureSurfaceTests.cs | 84 +++++++++++++++++++
.../DarlingWorker.cs | 8 ++
.../Mcp/DarlingMcpDataTools.cs | 2 +-
Lite/Mcp/McpHealthTools.cs | 2 +-
.../AlertReadFailureCounter.cs | 19 ++++-
5 files changed, 109 insertions(+), 6 deletions(-)
diff --git a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
index 45d012128..61b8b837a 100644
--- a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
+++ b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
@@ -383,6 +383,90 @@ public void EveryCountedSite_NamesItsReadDistinctly()
Assert.True(duplicates.Count == 0, $"duplicate read name(s): {string.Join(", ", duplicates)}");
}
+ ///
+ /// Every alert EVALUATION PASS records itself in the denominator.
+ ///
+ /// The census above proves each swallowed read is counted. It says nothing about whether the
+ /// pass that issued it is counted, and those are different claims: the PostgreSQL predictor group
+ /// shipped in review with all six of its read sites counted and no RecordPass at all, so a
+ /// PostgreSQL target reported two passes for three while its failures landed in the numerator
+ /// normally. A numerator guarded and a denominator unguarded is a worse instrument than neither,
+ /// because the pair still renders and now understates its own exposure.
+ ///
+ /// One pass per GROUP, not per check. The engine dispatches fourteen independently
+ /// failure-isolated Check*Async calls inside one pass and the predictor group dispatches six;
+ /// isolation granularity is not pass granularity. So the assertion is per entry point, and the
+ /// tree-wide count of RecordPass sites is asserted equal to the number of entry points so a
+ /// fourth pass added without recording itself fails here, and a RecordPass added somewhere that
+ /// is not a pass entry point fails too.
+ ///
+ [Fact]
+ public void EveryAlertEvaluationPass_RecordsItselfInTheDenominator()
+ {
+ /* (file, the member that IS the pass). Named rather than derived, like AlertPassCommandTimeoutTests'
+ own entry points: "is a pass" is a claim about dispatch that no pattern over source expresses. The
+ count assertion below is what makes the list safe. */
+ var passEntryPoints = new[]
+ {
+ (File: Path.Combine("PerformanceMonitor.Alerting", "AlertEngine.cs"), Member: "EvaluateCoreAsync"),
+ (File: Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingSelfAlertEvaluator.cs"),
+ Member: "EvaluateStoreAlertsAsync"),
+ (File: Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"),
+ Member: "EvaluatePostgresAlertsAsync"),
+ };
+
+ foreach (var (file, member) in passEntryPoints)
+ {
+ var raw = ReadSource(file);
+ var stripped = CSharpSourceWalker.StripCommentsAndStrings(raw);
+ var (start, end) = MemberBody(stripped, member);
+ var body = stripped[start..end];
+
+ Assert.True(
+ body.Contains("RecordPass(", StringComparison.Ordinal),
+ $"{member} in {Path.GetFileName(file)} is an alert evaluation pass that does not record "
+ + "itself, so every read failure it swallows lands in the numerator with nothing added to "
+ + "the denominator");
+ }
+
+ /* Both directions. A new pass that forgets to record fails the loop above; a RecordPass placed
+ anywhere that is not one of these entry points fails this count, which is what stops the
+ denominator from being padded by something that is not a pass. */
+ var recordPassSites = 0;
+ foreach (var file in new[]
+ {
+ Path.Combine("PerformanceMonitor.Alerting", "AlertReadFailureCounter.cs"),
+ Path.Combine("PerformanceMonitor.Alerting", "AlertEngine.cs"),
+ Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingSelfAlertEvaluator.cs"),
+ Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"),
+ Path.Combine("Lite", "MainWindow.xaml.cs"),
+ Path.Combine("Lite", "MainWindow.AlertEngine.cs"),
+ })
+ {
+ var stripped = CSharpSourceWalker.StripCommentsAndStrings(ReadSource(file));
+
+ /* Every mention, minus the declaration — which is not a call site. Counted as a plain
+ subtraction rather than as a lookbehind on the match, because doing BOTH excludes the
+ declaration twice and reports one call site fewer than exist. That is not hypothetical:
+ this pin's first run failed 2-against-3 on exactly that arithmetic, which is the reason
+ the count is asserted rather than the presence. */
+ recordPassSites += Regex.Matches(stripped, @"\bRecordPass\s*\(").Count
+ - Regex.Matches(stripped, @"void\s+RecordPass\s*\(").Count;
+ }
+
+ Assert.Equal(passEntryPoints.Length, recordPassSites);
+
+ /* And the SHIPPED note has to describe the inventory it now has, or the surface states a pass count
+ that stopped being true the moment a third pass was added — which is how this defect reached
+ review in the first place. */
+ Assert.Contains("runs three", AlertReadFailureCounter.WindowNote, StringComparison.Ordinal);
+ Assert.Contains("NOT across engines", AlertReadFailureCounter.WindowNote, StringComparison.Ordinal);
+ Assert.DoesNotContain(
+ "Darling runs two passes per sweep where Lite runs one",
+ AlertReadFailureCounter.WindowNote,
+ StringComparison.Ordinal);
+ }
+
/* ---------------- the surfaces ---------------- */
[Fact]
diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
index dae19edfa..949da0261 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
@@ -3225,6 +3225,14 @@ private async Task EvaluatePostgresAlertsAsync(
return;
}
+ /* #3013: the PostgreSQL predictor group is a THIRD alert evaluation pass, and it has to say so
+ or a PostgreSQL target's denominator reports two passes for three. One pass for the whole
+ group, not one per check: the six checks below are independently failure-isolated exactly as
+ AlertEngine's fourteen Check*Async calls are, and those fourteen are one pass. Isolation
+ granularity is not pass granularity. Recorded AFTER the guard above for the same reason the
+ engine records after its master switch — a pass that cannot reach the store is not one. */
+ _readFailures.RecordPass(snapshot.ServerKey);
+
try
{
var adapter = new DarlingPostgresAlertReadAdapter(_postgres);
diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
index 28c937bd8..5b1b97116 100644
--- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
@@ -884,7 +884,7 @@ internal static string RenderServerList(
}, McpHelpers.JsonOptions);
}
- [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server, instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
+ [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server (a pass is one alert evaluation pass containing many reads, so more failures than passes is ordinary and the pair is NOT a ratio; a Darling sweep runs two passes for a SQL Server target, three for a PostgreSQL one, and Lite runs one, so the denominator is comparable within a host and engine but not across them), instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
public static async Task GetCollectionHealth(
NpgsqlDataSource postgres,
[Description("Server name or display name.")] string? server_name = null)
diff --git a/Lite/Mcp/McpHealthTools.cs b/Lite/Mcp/McpHealthTools.cs
index 44510b1e9..53370aee2 100644
--- a/Lite/Mcp/McpHealthTools.cs
+++ b/Lite/Mcp/McpHealthTools.cs
@@ -203,7 +203,7 @@ and that difference is the most useful thing on this payload. */
}
}
- [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server, instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
+ [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server (a pass is one alert evaluation pass containing many reads, so more failures than passes is ordinary and the pair is NOT a ratio; a Darling sweep runs two passes for a SQL Server target, three for a PostgreSQL one, and Lite runs one, so the denominator is comparable within a host and engine but not across them), instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
public static async Task GetCollectionHealth(
LocalDataService dataService,
ServerManager serverManager,
diff --git a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
index 1ebc93eee..fc5fb1693 100644
--- a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
+++ b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
@@ -153,9 +153,16 @@ public void RecordReadFailure(string? serverKey, string readName)
/// anything. What the denominator is FOR is telling three failures over two hundred passes apart
/// from three over four.
///
- /// Nullable for symmetry with , but no caller passes null
- /// today: both SKUs' passes are per-server, and the fleet-scoped store self-alerts are polled on
- /// their own cadences rather than as a pass over a server.
+ ///
+ /// Three callers today, all per-server: the shared engine's EvaluateCoreAsync, Darling's
+ /// DarlingSelfAlertEvaluator.EvaluateStoreAlertsAsync, and Darling's PostgreSQL predictor group
+ /// DarlingWorker.EvaluatePostgresAlertsAsync. Each is one PASS containing many independently
+ /// failure-isolated checks — isolation granularity is not pass granularity, which is why the engine's
+ /// fourteen checks and the predictor group's six are one pass each rather than twenty.
+ /// Nullable for symmetry with , but no caller passes null today:
+ /// every pass is per-server, and the fleet-scoped store self-alerts are polled on their own cadences
+ /// rather than as a pass over a server.
+ ///
public void RecordPass(string? serverKey)
{
var bucket = string.IsNullOrWhiteSpace(serverKey) ? _fleet : Bucket(serverKey!);
@@ -309,5 +316,9 @@ public IReadOnlyList ServerKeys() =>
+ "fleet-scoped store self-alerts (disk pressure, compression-job health, store-job cadence, "
+ "retention holds), which belong to no server and so appear in no per-server count. "
+ "server_alert_passes is a denominator for judging whether the failure count is large, not a rate: a "
- + "pass issues many reads, and Darling runs two passes per sweep where Lite runs one.";
+ + "pass issues many reads, and the number of passes per sweep differs by host and by target engine: a "
+ + "Darling sweep of a SQL Server target runs two (the shared engine's conditions and the service's "
+ + "own store-polled self-alerts), a PostgreSQL target runs three (those two plus the PostgreSQL "
+ + "predictor group), and a Lite sweep runs one. So this denominator is comparable between servers "
+ + "on the same host and engine, and NOT across engines or across SKUs.";
}
From 75e3f02835572935349c64a6bba52981e462a318 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 15:57:59 -0400
Subject: [PATCH 07/11] 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.
---
.../AlertReadFailureSurfaceTests.cs | 21 ++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
index 61b8b837a..8ed1fda1b 100644
--- a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
+++ b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
@@ -458,9 +458,24 @@ the count is asserted rather than the presence. */
/* And the SHIPPED note has to describe the inventory it now has, or the surface states a pass count
that stopped being true the moment a third pass was added — which is how this defect reached
- review in the first place. */
- Assert.Contains("runs three", AlertReadFailureCounter.WindowNote, StringComparison.Ordinal);
- Assert.Contains("NOT across engines", AlertReadFailureCounter.WindowNote, StringComparison.Ordinal);
+ review in the first place.
+
+ All three arms of the inventory are asserted SEPARATELY and not by one phrase. A first draft
+ of this pin checked only that "runs three" appeared, and red-proofing found it green against a
+ note whose SQL Server arm had been broken — one true clause is not a true inventory. Each arm
+ is a distinct claim and each has to survive on its own. */
+ foreach (var arm in new[]
+ {
+ "SQL Server target runs two",
+ "PostgreSQL target runs three",
+ "Lite sweep runs one",
+ "NOT across engines",
+ })
+ {
+ Assert.Contains(arm, AlertReadFailureCounter.WindowNote, StringComparison.Ordinal);
+ }
+
+ /* The superseded claim, named so it cannot come back by a revert. */
Assert.DoesNotContain(
"Darling runs two passes per sweep where Lite runs one",
AlertReadFailureCounter.WindowNote,
From 60f31f1343116567436811c7100d92860b16b7dc Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 16:07:52 -0400
Subject: [PATCH 08/11] 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.
---
.../AlertReadFailureSurfaceTests.cs | 109 +++++++++++++++++-
1 file changed, 106 insertions(+), 3 deletions(-)
diff --git a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
index 8ed1fda1b..22909b0ef 100644
--- a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
+++ b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
@@ -202,7 +202,7 @@ private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScop
};
private const int WorkerCountedSites = 8;
- private const int WorkerExemptSites = 5;
+ private const int WorkerExemptSites = 6;
///
/// Log-message fragments that identify a catch block DELIBERATELY not counted, each paired with the
@@ -226,10 +226,24 @@ private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScop
["could not read pg_database_size"] = "context for the alert text, not the evidence the alert is judged on",
["Store self-metrics sweep did not finish"] = "a metrics write sweep; no alert is judged on its result",
["Recently-failed-job check errored"] = "reads the monitored server's msdb on its own connection and timeout",
+ ["Skipping recently-failed-job check"] = "the same msdb read, permission-denied arm; not a store read",
};
+ ///
+ /// ANY caught type, not just Exception. A census keyed on the one spelling it was written
+ /// for is #2786's failure, and this file walked into it: DarlingWorker.FetchFailedJobsAsync
+ /// swallows a failed msdb read in a catch (SqlException ex) when (…) filter, and the
+ /// Exception-only pattern could not see it. That one is exempt for its own stated reason, so
+ /// nothing was miscounted today — but a store read moved into a narrower catch inside a scoped
+ /// member would have reported CLEAN, which is the whole failure mode.
+ ///
+ /// OperationCanceledException is excluded, and proven rather than assumed: it is
+ /// cancellation propagation, not a swallowed read, and
+ /// asserts every one of those blocks in
+ /// scope either rethrows or logs nothing — so excluding them cannot hide a counted site.
+ ///
private static readonly Regex s_catch = new(
- @"catch\s*\(\s*(?:System\s*\.\s*)?Exception\b",
+ @"catch\s*\(\s*(?!OperationCanceledException\b)(?:System\s*\.\s*)?[A-Za-z_][A-Za-z0-9_.]*\b",
RegexOptions.Compiled | RegexOptions.CultureInvariant);
[Fact]
@@ -283,7 +297,7 @@ an off-by-one on a total. */
/* The whole-tree totals, so a site MOVED between the scoped regions still has to be re-counted by
a person rather than netting out silently. */
Assert.Equal(27, totalCounted);
- Assert.Equal(15, totalExempt);
+ Assert.Equal(16, totalExempt);
/* Every exemption in the table is actually used. An exemption for a message that no longer exists
is a hole this pin would otherwise keep open indefinitely — the shape that lets a real new catch
@@ -337,6 +351,39 @@ the third asserting the scan FAILS when it should. */
Assert.Equal(0, exempt);
Assert.Single(unclassified);
Assert.Contains("sprockets", unclassified[0], StringComparison.Ordinal);
+ unclassified.Clear();
+
+ /* A NARROWER caught type is still a catch. This is the arm that was missing: the scanner matched
+ only `Exception`, so a swallowed read behind `catch (SqlException ex) when (…)` inside a scoped
+ member was invisible to it. The `when` filter is carried in the fixture because that is the shape
+ that actually occurs. */
+ const string narrowFixture = """
+ try { Read(); }
+ catch (SqlException ex) when (IsPermissionDenied(ex.Number))
+ {
+ _logger.LogInformation("Skipping widget read: {Message}", ex.Message);
+ }
+ """;
+ (counted, exempt) = Classify(narrowFixture, CSharpSourceWalker.StripCommentsAndStrings(narrowFixture), "fixture", unclassified);
+ Assert.Equal(0, counted);
+ Assert.Equal(0, exempt);
+ Assert.Single(unclassified);
+ Assert.Contains("Skipping widget read", unclassified[0], StringComparison.Ordinal);
+ unclassified.Clear();
+
+ /* And a cancellation catch is deliberately NOT a census subject — excluded by the regex itself, with
+ NoCancellationCatch_QuietlySwallowsAReadFailure proving the exclusion cannot hide a counted site. */
+ const string cancelFixture = """
+ try { Read(); }
+ catch (OperationCanceledException)
+ {
+ throw;
+ }
+ """;
+ (counted, exempt) = Classify(cancelFixture, CSharpSourceWalker.StripCommentsAndStrings(cancelFixture), "fixture", unclassified);
+ Assert.Equal(0, counted);
+ Assert.Equal(0, exempt);
+ Assert.Empty(unclassified);
/* And a catch written only in PROSE is not a catch. The census reads stripped source for exactly
this reason — the exemption comments this change added to fourteen sites are prose, and a
@@ -482,6 +529,62 @@ is a distinct claim and each has to survive on its own. */
StringComparison.Ordinal);
}
+ ///
+ /// The census excludes catch (OperationCanceledException …). That exclusion is only safe while
+ /// those blocks never quietly swallow a read failure — so it is asserted rather than assumed.
+ ///
+ /// Every such block in the whole-file scopes must either rethrow (propagating cancellation, which
+ /// is not a failed read) or log nothing at error level. A block that logged an error and returned would
+ /// be a swallowed read hiding behind the one type the census does not look at — the same shape as the
+ /// narrower-catch gap that widening closed.
+ ///
+ [Fact]
+ public void NoCancellationCatch_QuietlySwallowsAReadFailure()
+ {
+ var offenders = new List();
+ var examined = 0;
+
+ var cancellationCatch = new Regex(
+ @"catch\s*\(\s*OperationCanceledException\b",
+ RegexOptions.Compiled | RegexOptions.CultureInvariant);
+
+ foreach (var (relative, _, _) in s_wholeFileScopes)
+ {
+ var raw = ReadSource(relative);
+ var stripped = CSharpSourceWalker.StripCommentsAndStrings(raw);
+
+ foreach (Match m in cancellationCatch.Matches(stripped))
+ {
+ var open = stripped.IndexOf('{', m.Index);
+ if (open < 0)
+ {
+ continue;
+ }
+
+ examined++;
+ var body = CSharpSourceWalker.BraceBalanced(stripped, open);
+
+ var rethrows = Regex.IsMatch(body, @"\bthrow\s*;");
+ var shouts = body.Contains("LogError", StringComparison.Ordinal)
+ || body.Contains("LogCritical", StringComparison.Ordinal);
+
+ if (!rethrows && shouts)
+ {
+ offenders.Add($"{Path.GetFileName(relative)} @offset {open}");
+ }
+ }
+ }
+
+ /* The precondition. A regex that matched nothing would make the assertion below vacuous, which is
+ exactly how an exclusion starts covering for something. */
+ Assert.True(examined >= 20, $"only {examined} cancellation catches were examined — the scan is not reaching them");
+
+ Assert.True(
+ offenders.Count == 0,
+ "cancellation catch block(s) log an error without rethrowing, so a swallowed read is hiding "
+ + $"behind the one caught type the census does not examine: {string.Join(", ", offenders)}");
+ }
+
/* ---------------- the surfaces ---------------- */
[Fact]
From 686c96961603c99747ffe62b1dba22fdf4d3ea18 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 16:14:02 -0400
Subject: [PATCH 09/11] 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.
---
.../AlertReadFailureSurfaceTests.cs | 38 ++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
index 22909b0ef..b1afc8437 100644
--- a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
+++ b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
@@ -462,6 +462,9 @@ count assertion below is what makes the list safe. */
Member: "EvaluatePostgresAlertsAsync"),
};
+ /* How many entry points the ORDER arm actually reached, so it cannot go vacuous unnoticed. */
+ var ordered = 0;
+
foreach (var (file, member) in passEntryPoints)
{
var raw = ReadSource(file);
@@ -469,13 +472,46 @@ count assertion below is what makes the list safe. */
var (start, end) = MemberBody(stripped, member);
var body = stripped[start..end];
+ var passAt = body.IndexOf("RecordPass(", StringComparison.Ordinal);
+
Assert.True(
- body.Contains("RecordPass(", StringComparison.Ordinal),
+ passAt >= 0,
$"{member} in {Path.GetFileName(file)} is an alert evaluation pass that does not record "
+ "itself, so every read failure it swallows lands in the numerator with nothing added to "
+ "the denominator");
+
+ /* ORDER, not just presence. A RecordPass placed INSIDE the try — after the reads rather than
+ before them — records the pass only on the cycles that succeeded, so a pass whose read
+ failed would contribute a failure to the numerator and nothing to the denominator. That is
+ the same defect as omitting the call, arriving through placement instead of absence, and
+ presence alone cannot see it. Asserted structurally rather than behaviourally because
+ reaching these bodies at runtime needs a live store, and a test that opens a socket to
+ prove an ordering is a flaky test proving a static fact.
+ Matched as the try STATEMENT rather than the substring: "try" occurs inside retry, entry and
+ geometry, and a substring hit would compare the pass against an arbitrary identifier.
+
+ Not every pass entry point HAS a try of its own — AlertEngine.EvaluateCoreAsync dispatches
+ fourteen checks that each own theirs — and where there is none the ordering claim does not
+ apply. The count of entry points the check actually reached is asserted below, so the arm
+ cannot quietly become vacuous for all three. */
+ var tryMatch = Regex.Match(body, @"\btry\s*\{");
+
+ if (tryMatch.Success)
+ {
+ ordered++;
+
+ Assert.True(
+ passAt < tryMatch.Index,
+ $"{member} records its pass at offset {passAt}, INSIDE or after the try at "
+ + $"{tryMatch.Index}: a cycle whose read fails would then add to the numerator and "
+ + "nothing to the denominator");
+ }
}
+ /* The order arm reached the two entry points that own their own try (the self-alert evaluator and
+ the PostgreSQL predictor group). A drop here means the arm stopped checking anything. */
+ Assert.True(ordered >= 2, $"the pass-ordering arm only reached {ordered} entry point(s)");
+
/* Both directions. A new pass that forgets to record fails the loop above; a RecordPass placed
anywhere that is not one of these entry points fails this count, which is what stops the
denominator from being padded by something that is not a pass. */
From d06dd69ffe74b68370466c74df96e475e5ae6543 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 16:31:03 -0400
Subject: [PATCH 10/11] 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.
---
.../AlertReadFailureSurfaceTests.cs | 78 +++++++++++++-
.../DarlingWorker.cs | 40 ++++++-
.../Mcp/DarlingMcpDataTools.cs | 2 +-
Lite.Tests/AlertReadFailureSurfaceTests.cs | 101 +++++++++++++++++-
Lite/Mcp/McpHealthTools.cs | 2 +-
PerformanceMonitor.Alerting/AlertEngine.cs | 11 +-
.../AlertReadFailureCounter.cs | 27 ++++-
7 files changed, 244 insertions(+), 17 deletions(-)
diff --git a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
index b1afc8437..4ee97698d 100644
--- a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
+++ b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs
@@ -148,7 +148,8 @@ output_note discipline applied to a different window. It also has to refuse the
"restart takes it to zero", /* why the zero can be small */
"deliberately not persisted", /* and why it is in memory */
"failed to DELIVER", /* the claim it refuses to make */
- "fleet-scoped store self-alerts", /* what the instance total covers */
+ "fleet-scoped conditions", /* what the instance total covers */
+ "collector-cost regression", /* named because it is counted and was omitted */
"not a rate", /* what the denominator is not */
})
{
@@ -169,7 +170,7 @@ real failure rather than a matcher that never matches anything. */
///
private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScopes =
{
- (Path.Combine("PerformanceMonitor.Alerting", "AlertEngine.cs"), 14, 3),
+ (Path.Combine("PerformanceMonitor.Alerting", "AlertEngine.cs"), 13, 4),
(Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingSelfAlertEvaluator.cs"), 5, 7),
};
@@ -201,7 +202,7 @@ private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScop
"FetchFailedJobsAsync",
};
- private const int WorkerCountedSites = 8;
+ private const int WorkerCountedSites = 9;
private const int WorkerExemptSites = 6;
///
@@ -227,6 +228,7 @@ private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScop
["Store self-metrics sweep did not finish"] = "a metrics write sweep; no alert is judged on its result",
["Recently-failed-job check errored"] = "reads the monitored server's msdb on its own connection and timeout",
["Skipping recently-failed-job check"] = "the same msdb read, permission-denied arm; not a store read",
+ ["Failed to check failed jobs"] = "the fetcher reads the monitored server's msdb; the block's only store op is a write both stores swallow",
};
///
@@ -297,7 +299,7 @@ an off-by-one on a total. */
/* The whole-tree totals, so a site MOVED between the scoped regions still has to be re-counted by
a person rather than netting out silently. */
Assert.Equal(27, totalCounted);
- Assert.Equal(16, totalExempt);
+ Assert.Equal(17, totalExempt);
/* Every exemption in the table is actually used. An exemption for a message that no longer exists
is a hole this pin would otherwise keep open indefinitely — the shape that lets a real new catch
@@ -621,6 +623,74 @@ public void NoCancellationCatch_QuietlySwallowsAReadFailure()
+ $"behind the one caught type the census does not examine: {string.Join(", ", offenders)}");
}
+ ///
+ /// A counted read must not share a try with the dispatch of a pass, or the pass is unreachable
+ /// exactly when that read fails — numerator up, denominator unchanged.
+ ///
+ /// The third route to the denominator defect, and the one neither of the other two arms can see.
+ /// covers OMISSION (no
+ /// RecordPass) and PLACEMENT (the call sitting after the reads it should precede). This covers
+ /// REACHABILITY: a RecordPass that is present, correctly placed inside its own method, and simply
+ /// never entered because an earlier statement in the CALLER's try threw first.
+ ///
+ /// It was real. DarlingWorker.EvaluateAlertsAsync read the latest CPU sample — a store read
+ /// on the alert-pass deadline, so the first read to fail under the contention #3013 measures — inside the
+ /// same try as engine.EvaluateServerAsync. A failed CPU read skipped the engine sweep entirely, so
+ /// EvaluateCoreAsync never recorded its pass while the caller's catch still recorded a failure. It
+ /// also cost the server every other condition that tick, which was the larger half.
+ ///
+ [Fact]
+ public void NoCountedRead_SharesATryWithThePassItWouldSkip()
+ {
+ var raw = ReadSource(Path.Combine(
+ "Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"));
+ var stripped = CSharpSourceWalker.StripCommentsAndStrings(raw);
+ var (start, end) = MemberBody(stripped, "EvaluateAlertsAsync");
+ var body = stripped[start..end];
+
+ /* The pass dispatches this method performs, and the counted reads it performs itself. Both must be
+ present or the pin is describing a method that no longer exists. */
+ var dispatch = body.IndexOf("engine.EvaluateServerAsync", StringComparison.Ordinal);
+ var cpuRead = body.IndexOf("ReadLatestCpuAsync", StringComparison.Ordinal);
+
+ Assert.True(dispatch > 0, "EvaluateAlertsAsync no longer dispatches the shared engine sweep");
+ Assert.True(cpuRead > 0, "EvaluateAlertsAsync no longer performs the latest-CPU read");
+
+ /* Which try block, if any, encloses each. Computed by brace-balancing every try in the body rather
+ than by comparing offsets to a single try, because this method now has two and the whole point is
+ that these two statements are in different ones. */
+ var enclosing = new List<(int Index, int Start, int End)>();
+ foreach (Match m in Regex.Matches(body, @"\btry\s*\{"))
+ {
+ var open = body.IndexOf('{', m.Index);
+ var block = CSharpSourceWalker.BraceBalanced(body, open);
+ enclosing.Add((enclosing.Count, open, open + block.Length));
+ }
+
+ Assert.True(enclosing.Count >= 2, $"EvaluateAlertsAsync has {enclosing.Count} try block(s); the CPU read is no longer isolated from the sweep");
+
+ int TryOf(int offset)
+ {
+ foreach (var (index, s2, e2) in enclosing)
+ {
+ if (offset > s2 && offset < e2)
+ {
+ return index;
+ }
+ }
+
+ return -1;
+ }
+
+ var cpuTry = TryOf(cpuRead);
+ var dispatchTry = TryOf(dispatch);
+
+ Assert.NotEqual(-1, dispatchTry);
+ Assert.NotEqual(
+ cpuTry,
+ dispatchTry);
+ }
+
/* ---------------- the surfaces ---------------- */
[Fact]
diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
index 949da0261..35636dbc4 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
@@ -3173,9 +3173,45 @@ private async Task EvaluateAlertsAsync(
return;
}
+ /* #3013: the latest-CPU read is isolated from the sweep it feeds, for TWO reasons that point the
+ same way.
+
+ Correctness of the instrument: this read runs on AlertPassCommandTimeoutSeconds and is the
+ first store read of the pass, so under the contention #3013 measures it is the first to fail.
+ Inside the sweep's try it took engine.EvaluateServerAsync down with it, which meant
+ AlertEngine.EvaluateCoreAsync never ran and never recorded its pass - while the catch below
+ still recorded a read failure. Numerator up, denominator unchanged, worst exactly when the
+ counter matters most. That is a third route to the same defect the PostgreSQL predictor group
+ had: not omission and not placement, but REACHABILITY - a pass site that is real and
+ correctly placed and simply never entered.
+
+ Correctness of the ALERTING, which is the bigger half: a single failed CPU read aborted the
+ whole shared sweep for this server this tick, so blocking, deadlocks, poison waits,
+ long-running queries, TempDB, low disk, PVS, file growth, jobs, database state and forced
+ plans were none of them evaluated. The snapshot already documents a null CPU pair as a normal
+ input ("null when no SQL sample") and CheckCpuAsync gates on alertCpuValue.HasValue, so
+ degrading to (null, null) costs this tick its CPU alert and nothing else. */
+ double? sqlCpu = null;
+ double? totalCpu = null;
+
+ try
+ {
+ (sqlCpu, totalCpu) = await ReadLatestCpuAsync(runtime.ServerId, cancellationToken);
+ }
+ catch (OperationCanceledException)
+ {
+ throw;
+ }
+ catch (Exception ex)
+ {
+ _logger.LogError("[{Server}] Latest-CPU read for the alert pass failed: {Message}",
+ server.Config.DisplayName, ex.Message);
+ _readFailures.RecordReadFailure(
+ runtime.ServerId.ToString(CultureInfo.InvariantCulture), "latest-CPU read");
+ }
+
try
{
- var (sqlCpu, totalCpu) = await ReadLatestCpuAsync(runtime.ServerId, cancellationToken);
var snapshot = new AlertServerSnapshot(
runtime.ServerId.ToString(CultureInfo.InvariantCulture),
runtime.Config.DisplayName,
@@ -3206,7 +3242,7 @@ PostgreSQL read. */
_logger.LogError("[{Server}] Alert sweep failed: {Message}", server.Config.DisplayName, ex.Message);
_readFailures.RecordReadFailure(
runtime.ServerId.ToString(CultureInfo.InvariantCulture),
- "alert pass (latest-CPU read and the shared engine sweep)");
+ "shared engine sweep");
}
}
diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
index 5b1b97116..a9018b282 100644
--- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs
@@ -884,7 +884,7 @@ internal static string RenderServerList(
}, McpHelpers.JsonOptions);
}
- [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server (a pass is one alert evaluation pass containing many reads, so more failures than passes is ordinary and the pair is NOT a ratio; a Darling sweep runs two passes for a SQL Server target, three for a PostgreSQL one, and Lite runs one, so the denominator is comparable within a host and engine but not across them), instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
+ [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server (a pass is one alert evaluation pass containing many reads, so more failures than passes is ordinary and the pair is NOT a ratio; a Darling sweep runs two passes for a SQL Server target, three for a PostgreSQL one, and Lite runs one, so the denominator is comparable within a host and engine but not across them), instance_read_failures for the whole service (which also covers the fleet-scoped conditions that belong to no server and so appear in no per-server count: " + AlertReadFailureCounter.FleetScopedReads + "), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
public static async Task GetCollectionHealth(
NpgsqlDataSource postgres,
[Description("Server name or display name.")] string? server_name = null)
diff --git a/Lite.Tests/AlertReadFailureSurfaceTests.cs b/Lite.Tests/AlertReadFailureSurfaceTests.cs
index 192a7bfc6..be0eb4588 100644
--- a/Lite.Tests/AlertReadFailureSurfaceTests.cs
+++ b/Lite.Tests/AlertReadFailureSurfaceTests.cs
@@ -191,6 +191,68 @@ an unchanged string agreeing with each other. */
Assert.Contains("failed to DELIVER", descriptions[0], StringComparison.Ordinal);
}
+ ///
+ /// The fleet-scoped inventory is what the counter actually records, in both directions.
+ ///
+ /// The hand-maintained version of this list was wrong twice over: it named store DISK PRESSURE,
+ /// whose two feed reads are both exempt so it can never contribute a failure, and it omitted the
+ /// collector-cost regression self-alert, which does. Both errors point the same way for an operator —
+ /// a nonzero instance total, every server at zero, and a documented list that does not name the cause.
+ ///
+ /// So the set is derived from SOURCE (every RecordReadFailure(null, ...) call, matched
+ /// across line breaks because those calls are wrapped) and each one must be represented in the single
+ /// constant every surface now concatenates. A sixth fleet-scoped site fails here until the constant
+ /// names it.
+ ///
+ [Fact]
+ public void TheFleetScopedInventory_MatchesWhatTheCounterActuallyRecords()
+ {
+ var root = RepoRoot();
+ var nullKeyReads = new List();
+
+ foreach (var relative in new[]
+ {
+ Path.Combine("PerformanceMonitor.Alerting", "AlertEngine.cs"),
+ Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingSelfAlertEvaluator.cs"),
+ Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"),
+ })
+ {
+ var src = File.ReadAllText(Path.Combine(root, relative));
+
+ /* Singleline, because both of these calls are wrapped across lines — a line-bound pattern found
+ one of the two and would have "proved" a single fleet-scoped site. */
+ foreach (Match m in Regex.Matches(src, @"RecordReadFailure\(\s*null\s*,\s*""([^""]+)""", RegexOptions.Singleline))
+ {
+ nullKeyReads.Add(m.Groups[1].Value);
+ }
+ }
+
+ Assert.Equal(2, nullKeyReads.Count);
+
+ var inventory = AlertReadFailureCounter.FleetScopedReads;
+
+ /* Each recorded site is represented. Keyed on the distinguishing word rather than the whole read
+ name, because the constant is prose for an operator and the read name is a label for a log. */
+ Assert.Contains(nullKeyReads, r => r.Contains("collector-cost", StringComparison.Ordinal));
+ Assert.Contains(nullKeyReads, r => r.Contains("background-job health", StringComparison.Ordinal));
+ Assert.Contains("collector-cost regression", inventory, StringComparison.Ordinal);
+ Assert.Contains("background-job health", inventory, StringComparison.Ordinal);
+
+ /* And the phantom stays gone. Disk pressure's feed reads are exempt — a local filesystem read and a
+ pg_database_size read that is context for the alert text — so naming it here would send an
+ operator after a read that cannot fail into this number. */
+ Assert.DoesNotContain("disk pressure", inventory, StringComparison.OrdinalIgnoreCase);
+
+ /* The constant is what every surface concatenates, so this is also the cross-surface tie. */
+ foreach (var file in CollectionHealthToolFiles())
+ {
+ Assert.Contains(
+ "AlertReadFailureCounter.FleetScopedReads",
+ File.ReadAllText(file),
+ StringComparison.Ordinal);
+ }
+ }
+
/* ---------------- helpers ---------------- */
/// The field names inside a tool's alert_read_health = new { … } initializer.
@@ -238,16 +300,47 @@ private static SortedSet AlertReadFieldNames(string source)
return names;
}
+ ///
+ /// The tool's description as the CLIENT sees it, reassembled from however the literal is spelled.
+ ///
+ /// It is no longer one literal: both descriptions concatenate
+ /// so the fleet-scoped set cannot drift
+ /// between them. A pattern that captured a single quoted run would simply stop matching, which is at
+ /// least loud — but it would also stop comparing the halves either side of the constant, so the
+ /// segments are concatenated and the constant substituted in its place.
+ ///
private static string ToolDescription(string source)
{
- var m = Regex.Match(
+ var call = Regex.Match(
source,
- @"\[McpServerTool\(Name = ""get_collection_health""\), Description\(""(.*?)""\)\]",
+ @"\[McpServerTool\(Name = ""get_collection_health""\), Description\((.*?)\)\]",
RegexOptions.Singleline);
- Assert.True(m.Success, "a get_collection_health tool has no Description attribute in the expected shape");
+ Assert.True(call.Success, "a get_collection_health tool has no Description attribute in the expected shape");
+
+ var assembled = new System.Text.StringBuilder();
+ foreach (var piece in Regex.Split(call.Groups[1].Value, @"\s*\+\s*"))
+ {
+ var trimmed = piece.Trim();
+
+ if (trimmed.StartsWith("\"", StringComparison.Ordinal) && trimmed.EndsWith("\"", StringComparison.Ordinal))
+ {
+ assembled.Append(trimmed[1..^1]);
+ }
+ else if (trimmed.EndsWith("FleetScopedReads", StringComparison.Ordinal))
+ {
+ assembled.Append(AlertReadFailureCounter.FleetScopedReads);
+ }
+ else
+ {
+ Assert.Fail($"unrecognised piece in the Description concatenation: {trimmed}");
+ }
+ }
+
+ var text = assembled.ToString();
+ Assert.True(text.Length > 5000, $"the reassembled description is only {text.Length} chars — the split lost content");
- return m.Groups[1].Value;
+ return text;
}
private static string ReadSource(string relative)
diff --git a/Lite/Mcp/McpHealthTools.cs b/Lite/Mcp/McpHealthTools.cs
index 53370aee2..56c07b764 100644
--- a/Lite/Mcp/McpHealthTools.cs
+++ b/Lite/Mcp/McpHealthTools.cs
@@ -203,7 +203,7 @@ and that difference is the most useful thing on this payload. */
}
}
- [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server (a pass is one alert evaluation pass containing many reads, so more failures than passes is ordinary and the pair is NOT a ratio; a Darling sweep runs two passes for a SQL Server target, three for a PostgreSQL one, and Lite runs one, so the denominator is comparable within a host and engine but not across them), instance_read_failures for the whole service (which also covers the fleet-scoped store self-alerts - disk pressure, compression-job health, store-job cadence, retention holds - that belong to no server and so appear in no per-server count), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
+ [McpServerTool(Name = "get_collection_health"), Description("Shows the health status of all data collectors for a server — whether they're running successfully, failing, or stale. A collector reads STOPPED rather than FAILING when it has attempted nothing at all — no success, no error, nothing — for longer than the FAILING cutoff, despite a history of runs: that is a collector whose gate (AppliesTo) flipped off for this target rather than one that keeps running and erroring, and it does not count toward a server's failing-collector total. Check this before investigating data to ensure collectors are working properly. Each row also carries last_note/note_count: what a NON-failing run reported, e.g. an enumeration that came back with 0 items. note_count equal to total_runs means the collector has been collecting nothing all window — not a fault (the target may be legitimately empty), but the reason a HEALTHY collector can still have no data. target_has_user_databases tells those two apart: true means the target DID have user databases in the same window, so an all-window empty enumeration is worth investigating (a login that cannot enter them, an exclusion filter that matched everything); false means either no user databases or no inventory to go on. Each row also carries abandoned and abandon_rate_pct: cycles the 120-second whole-server wall-clock budget gave up on, which stored nothing and advanced no watermark. Unlike a yield, which retries, an abandoned cycle is collected data you do not have. A rate above 0.5% bands the collector WARNING, so a WARNING here may have nothing to do with errors - read abandoned beside errors to attribute it. CRITICAL for reading last_error: it is a single slot carrying the newest ERROR or PERMISSIONS message in the whole window, and a message in it is NOT evidence that the condition is current. Read last_error_at for when it happened, last_denied_at for when the newest DENIAL specifically happened, and denied_since_last_success for the derived answer - true means a denial is the collector's current state, false means every denial in the window predates a later success and the collector is reading fine now. A fault recorded before a code path changed will sit in last_error for the rest of the window while every cycle since succeeds: pg_deadlocks moved from an in-database route to an AWS API route, and six days later this tool still showed HEALTHY, errors 0, a reassuring note and a stale permission denial together - a combination that describes a state which cannot occur, and which produced a bug report claiming a fleet-wide denial when the collector had been succeeding on all 50 targets. Do not infer a live condition from last_error alone. Total abandonment still reads FAILING through staleness; the rate exists for the partial case, where a collector abandons some cycles and succeeds often enough to stay fresh, which otherwise read HEALTHY with errors 0 indefinitely. The sweep_pressure block is the server-level roll-up: it compares the collectors' combined execution demand (average duration amortized by cadence) against the minute the fastest cadence holds. SATURATED means the collection body cannot fit inside its cadence, so relaunches are skipped and the server collects at a multiple of its configured interval while every collector still reads healthy — heaviest_collectors names where that budget goes. That verdict is the SUSTAINED answer only. peak_cycle_risk is the separate single-sweep answer: peak_cycle_ms is what the body costs on the cycle where every scheduled cadence comes due together, and BODY_OVERRUN means that one body cannot fit the budget even when the verdict reads OK — the signature of one infrequent heavy collector, which amortization hides and heaviest_collectors therefore ranks out of sight. peak_collector names it, and peak_cycle_note explains it. Read both fields: a server can be OK/BODY_OVERRUN (a schedule-shape problem, fix by moving or splitting that collector) or SATURATED/BODY_OVERRUN (a capacity problem). Every collector row carries avg_duration_ms, p95_duration_ms and max_duration_ms, because a collector's runs are not always one population: query_store on one dogfood server averaged 13,834 ms over 1,155 runs of which 958 yielded nothing and cost about 36 ms, which puts the other 197 at roughly 80,900 ms EACH - each one, on its own, larger than the whole sweep budget. Read the three together: avg close to p95 close to max is one population, avg far below p95 is two, and p95 far below max is one pathological run. peak_cycle_ms is built from p95 (floored at the mean, so it can never read lower than a mean-based figure) for exactly that reason, and peak_collector carries peak_run_ms beside avg_duration_ms so the gap is visible. Those three still describe RUNS, and a collector that runs once per DATABASE writes one blended row, so no run-level statistic can say which database cost what. Five fan out from an enumeration on any SQL Server target (query_store, plan_correction, query_store_health, index_object_stats, database_scoped_config); separately, eight more fan out over a per-database connection loop when the target is Azure SQL DB, and pg_autovacuum_stats always does on PostgreSQL. The per-collector `fanout` block is that answer, null for a collector that does not fan out: `items` is how wide the fan-out was, `slowest`/`slowest_ms` name the dearest database and its cost on the window's worst run, `run_ms` is that whole run, and `dominance` is slowest_ms * items / run_ms — 1.0 for a perfectly even fan-out, rising with concentration. It matters because the remedies diverge there: near 1.0 the cost is the fan-out's WIDTH and bounded parallelism is the lever, while around 2.0 or above one database dominates and a per-database schedule override or a stagger is what helps. Do not try to infer this from p95 versus avg — on a per-database collector that ratio is usually saturated by empty-versus-productive runs and says nothing about databases. Every field named so far describes what a collector SPENT; rows_stored, runs_with_rows and productive_run_pct are what it BOUGHT, counted over the same window as total_runs and the durations, so cost and output on a row always describe the same runs. Read them together for the three readings that need different actions: rows_stored above zero is expensive AND productive; rows_stored zero with denied_since_last_success false is a collector that read and found nothing, which for one that stores a row only when an event occurs (e.g. deadlocks, blocked_process_report, pg_blocking, pg_xmin_horizon) is the correct resting state and needs no action; rows_stored zero with denied_since_last_success true is a collector that could not read and needs a grant. output_finding says which of the two zero readings applies and is null whenever rows_stored is positive. This is deliberately NOT a band: pg_deadlocks was the single most expensive collector on one managed store, 49,258,335 ms over 79,333 runs in seven days, and stored zero rows - and that zero was CORRECT, because the reader was working on all 50 targets and there were no deadlocks to find. A verdict keyed on cost-plus-zero-rows would fire on the healthy quiet install rather than the blind one. These are NOT the hourly per-collector series Darling's get_collector_cost reports as total_rows - a separate series over that caller's own days_back and across every server at once, and Darling-only, so Lite has no twin of it; the top-level output_note names both windows and disclaims that one. rows_stored is also what a run STORED, never what the monitored engine counted, so a zero cannot tell a genuinely quiet source apart from a reader capturing nothing off a busy one - nothing on this surface measures that. One block on this response is deliberately NOT on the seven-day window: alert_read_health, which counts the alerting layer's OWN store reads that failed and were swallowed. A condition check that cannot read the store logs one line and skips - correctly, because firing on absent evidence would fabricate an alert and resolving on it would fabricate a recovery - and that skip is not a collector run, so it writes no collection_log row and reaches no other health surface: only a grep of the service log found the class. It matters out of proportion to the count because the alert pass runs on a much shorter store deadline than the collection sweep, so as store latency rises the alerting layer is the FIRST thing to fail and collection is the last - during one measured episode of store lock contention the service log's error rate rose 41 to 61 per hour, every line an alerting-side read, while collector failures over the same hours FELL from 23 to 2. Read server_read_failures beside server_alert_passes for this server (a pass is one alert evaluation pass containing many reads, so more failures than passes is ordinary and the pair is NOT a ratio; a Darling sweep runs two passes for a SQL Server target, three for a PostgreSQL one, and Lite runs one, so the denominator is comparable within a host and engine but not across them), instance_read_failures for the whole service (which also covers the fleet-scoped conditions that belong to no server and so appear in no per-server count: " + AlertReadFailureCounter.FleetScopedReads + "), last_failure_read for which condition went blind most recently, and last_failure_at to tell a healed episode from a live one: this count never ages out of a window, so a nonzero value with a stamp from days ago is history. counting_since is when this process began counting, early in its own startup - these are in-memory counts and a restart takes them to zero, so a zero means \"none since counting_since\" and NOT \"none in seven days\"; check the stamp before reading the zero as reassurance. Deliberately not persisted, because what it counts is a failure to READ the store. It does NOT count alerts that failed to DELIVER and makes no claim about them - that is get_alert_history's question. And deliberately not a band, for the same reason the output figures are not: any threshold over it would have to guess how many blind reads make alerting unhealthy, and a wrong guess on this particular surface fails by saying nothing is wrong.")]
public static async Task GetCollectionHealth(
LocalDataService dataService,
ServerManager serverManager,
diff --git a/PerformanceMonitor.Alerting/AlertEngine.cs b/PerformanceMonitor.Alerting/AlertEngine.cs
index 9d3c0128a..6d7612750 100644
--- a/PerformanceMonitor.Alerting/AlertEngine.cs
+++ b/PerformanceMonitor.Alerting/AlertEngine.cs
@@ -1566,8 +1566,17 @@ await FireAsync(new AlertOutcome(
}
catch (Exception ex)
{
+ /* NOT counted by #3013's swallowed-read counter, and the only counted-looking site that is
+ deliberately not counted. Nothing reachable in this try is a STORE read. The fetcher reads
+ the MONITORED SERVER's msdb over its own connection and timeout - the same population
+ DarlingWorker.FetchFailedJobsAsync is exempted for, and counting it here while exempting it
+ there would put a target-side outage into a number an operator reads as store contention.
+ The one store operation in the block is SaveFailedJobWatermarkAsync, a WRITE, and both
+ implementations swallow it (PgAlertStateStore and DuckDbAlertHistoryStore each log "Could
+ not persist failed-job watermark" without rethrowing), so it cannot reach this catch at
+ all. The write sitting inside this try where the blocking, deadlock and database-state
+ checks keep theirs outside is a real asymmetry and an inert one. */
_logger?.LogError("Failed to check failed jobs for {Server}: {Message}", serverName, ex.Message); /* :715 */
- _readFailures?.RecordReadFailure(key, "failed jobs");
}
return conditionPresent;
diff --git a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
index fc5fb1693..bc10edb10 100644
--- a/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
+++ b/PerformanceMonitor.Alerting/AlertReadFailureCounter.cs
@@ -55,8 +55,8 @@ namespace PerformanceMonitor.Alerting;
/// serverId.ToString(CultureInfo.InvariantCulture); Lite: serverId.ToString()). Both are
/// same-process reads of the same rendering, so they agree by construction — and
/// AlertReadFailureSurfaceTests pins the agreement from source rather than trusting it. Failures
-/// belonging to no server (the fleet-scoped store self-alerts — disk pressure, compression-job health,
-/// store-job cadence, retention holds) are recorded with a null key: they land in the instance total and
+/// belonging to no server (the fleet-scoped conditions names) are
+/// recorded with a null key: they land in the instance total and
/// in no server's count, which is why the surface reports BOTH numbers. A per-server-only figure would
/// have given those conditions no home at all, reproducing #3013's own defect one level down.
///
@@ -291,6 +291,25 @@ public IReadOnlyList ServerKeys() =>
which);
}
+ ///
+ /// The conditions whose swallowed reads belong to NO server, named once so the note, the class
+ /// remarks and both SKUs' tool descriptions cannot disagree about the set.
+ ///
+ /// Referenced rather than repeated: both get_collection_health descriptions
+ /// concatenate this constant into their Description attribute, which is legal because a
+ /// const-string concatenation is a compile-time constant — so the set cannot grow in one place and
+ /// go stale in three. The first draft hand-maintained it and was wrong in both directions at once:
+ /// it listed store DISK PRESSURE, whose two feed reads are both exempt (a local filesystem read,
+ /// and a pg_database_size read that is context for the alert text rather than the evidence
+ /// it is judged on), so that condition can never contribute a failure here; and it omitted the
+ /// collector-cost regression self-alert, which does. An operator reading the phantom list would
+ /// have hunted a disk-pressure read that cannot fail into this number, and would not have thought
+ /// to check the one that can.
+ ///
+ public const string FleetScopedReads =
+ "the collector-cost regression self-alert, and the store background-job health reads behind "
+ + "compression-job health, store-job cadence and retention holds";
+
///
/// The window these figures cover, and the window they do NOT.
///
@@ -313,8 +332,8 @@ public IReadOnlyList ServerKeys() =>
+ "surface: they are not collector runs and write no collection_log row. It does NOT count fired "
+ "alerts that failed to DELIVER, and it makes no claim about them — that is the alert-history read's "
+ "question, not this one. instance_read_failures spans every server on this service plus the "
- + "fleet-scoped store self-alerts (disk pressure, compression-job health, store-job cadence, "
- + "retention holds), which belong to no server and so appear in no per-server count. "
+ + "fleet-scoped conditions that belong to no server and so appear in no per-server count: "
+ + FleetScopedReads + ". "
+ "server_alert_passes is a denominator for judging whether the failure count is large, not a rate: a "
+ "pass issues many reads, and the number of passes per sweep differs by host and by target engine: a "
+ "Darling sweep of a SQL Server target runs two (the shared engine's conditions and the service's "
From e022af1d2e4c25f598fbe4fb7a4adc4d0a8c796c Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sat, 5 Sep 2026 16:39:25 -0400
Subject: [PATCH 11/11] 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.
---
.../DarlingWorker.cs | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
index 35636dbc4..cb05e1882 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
@@ -3265,8 +3265,19 @@ private async Task EvaluatePostgresAlertsAsync(
or a PostgreSQL target's denominator reports two passes for three. One pass for the whole
group, not one per check: the six checks below are independently failure-isolated exactly as
AlertEngine's fourteen Check*Async calls are, and those fourteen are one pass. Isolation
- granularity is not pass granularity. Recorded AFTER the guard above for the same reason the
- engine records after its master switch — a pass that cannot reach the store is not one. */
+ granularity is not pass granularity. Recorded after the guard above because a pass that cannot
+ reach the store is not one.
+
+ NOT parity with the other two sites, and an earlier version of this comment wrongly claimed it
+ was. The shared engine records its pass after AlertEngine.EvaluateServerAsync's
+ !_settings.AlertsEnabled early return, and DarlingSelfAlertEvaluator.EvaluateStoreAlertsAsync
+ returns before recording on the same check. This path has no master-switch gate at all —
+ DarlingWorker holds no reference to AlertsEnabled anywhere — so with alerting switched off the
+ PostgreSQL predictors still read, still evaluate and still reach _alertDeliverer, and this
+ counts the pass that really did run. That gap pre-dates this counter and is a question about
+ whether the Tier 0 predictors are deliberately exempt from the switch, not something to settle
+ by gating a denominator: gating only this line would make the count deny passes that happened
+ and alerts that fired. The count stays truthful and the gap stays named. */
_readFailures.RecordPass(snapshot.ServerKey);
try