From 7ac940cf3463af42dc7da9ef040f2b459abf7708 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 13:32:51 +0100 Subject: [PATCH 1/3] Make Darling's MCP alert payload and its writer agree (#2417) get_alert_settings and update_alert_settings are the read and write halves of one file, and they had drifted apart in both directions at once. Six columns AlertSettingsReadRow reads were emitted to nobody. The reader was widened for them deliberately -- its own comment says so, "an MCP client could not see the V33 connection opt-ins or the V35 Availability Group family at all" -- but the payload never followed, so every get_alert_settings call pulled notify_ag_health, ag_lag_alert_seconds, ag_redo_queue_alert_kb, ag_disconnect_refire_minutes, notify_connection_down_at_startup and connection_refire_minutes out of the store and dropped them on the floor. The AG four are the ones that cost something: those are the settings whose misconfiguration is least visible anywhere else, so an agent asked why nothing alerted when a replica fell behind could not see the lag threshold, could not see whether AG notification was on at all, and had no way to tell "configured not to alert" apart from "failed to alert". In the other direction the payload emitted blocking.wait_threshold_seconds, which the blocking arm of the update switch did not accept. So handing a whole read payload back to the writer -- the round trip the tool's own description tells the caller to perform, read the settings, change one number, write them back -- was rejected with "Unknown field", naming a field the caller never chose to send. All six are now WRITABLE, not merely readable. Every column on this row is a knob the Viewer's Settings window already writes, so a read-only knob just moves an operator to a hand-typed UPDATE; and read-only would have needed an exemption entry in the new test, which is the same hand-maintained fact that let the six go missing. Every bound mirrors DarlingAlertSettings' clamp exactly -- Clamp(0,1440) on both re-fires, Clamp(0,86400) on the lag, Clamp(0L,1073741824L) on the redo queue, Math.Max(0,...) on the blocking wait -- because a bound that differs lets the tool accept a value the engine silently rewrites on read, which presents as the setting not sticking, and nothing says no. Zero stays in range everywhere: on all four numerics zero is the shipped configuration meaning that gate is off, and the redo queue ships at zero precisely because a healthy queue size is workload-specific. The redo queue is bound through a new AddLong rather than AddInt because it is the one bigint on the row and its ceiling only happens to fit in an int today. The wire keys are the house's, not transliterations of the column names. The AG family is a group like every other alert family, with its master switch named ag.enabled for the same reason cpu/blocking/pvs/database_state spell theirs that way -- inside this payload "enabled" always means this alert family is on, and notify_ag_health IS that switch rather than a second opt-in behind one. Its two thresholds take the _threshold_ spelling the neighbours already use (blocking.wait_threshold_seconds, poison_wait.threshold_ms, low_disk.threshold_gb) instead of the columns' older _alert_ suffix, which appears nowhere on the wire. The two connection settings stay top-level beside notify_connection_changes rather than becoming a connection group: the master shipped as a top-level key and Lite emits it there too, so a group could only ever have held two of the three, and splitting one family across two levels of the document is worse for a reader than two extra top-level keys. Their spellings are the store's column names, which are also the keys Lite already reads out of settings.json, so Lite's payload can adopt them verbatim when it follows. The part worth keeping is the test. The existing ones pin individual keys, which is exactly why six missing keys and one unroundtrippable key both survived, so the new one is derived instead: it builds a settings row, runs it through the real BuildAlertSettingsPayload, serializes it through the same options the tool serializes with, and feeds the result to the real BuildAlertSettingsUpdate -- then asserts the set of columns that comes out equals the set of columns AlertSettingsSelectSql reads. One equality closes all three directions. A column read but not emitted is missing from the write set; a key emitted but refused shows up as a parse error naming itself; and a key emitted for a column nobody reads fails the direction no hand-maintained list would ever have noticed. There are no exemptions, deliberately -- if a genuinely read-only column arrives it gets exempted by name with its reason, rather than the equality being loosened. Darling.Tests targets net10.0-windows and cannot run on macOS, so the invariant was executed against the real compiled assemblies through a throwaway net10.0 harness. It passes on this branch and fails on dev twice over: with dev's file it stops at "Unknown field 'blocking.wait_threshold_seconds'", and with dev's file plus only the blocking arm added it names all six missing columns and nothing else. Common gains an InternalsVisibleTo for Darling.Tests so the test serializes through McpHelpers.JsonOptions itself rather than a look-alike; the two sibling test projects were already on that list. Nothing about the SELECT or the by-ordinal read changed -- every column was already being read, which is what made this a payload defect rather than a schema one, so AlertSettingsSelect_ColumnCount_MatchesTheOrdinalsRead is untouched and still holds. Co-Authored-By: Claude Opus 5 --- .../DarlingMcpAlertToolsTests.cs | 168 ++++++++++++++++++ .../Mcp/DarlingMcpAlertTools.cs | 85 ++++++++- .../PerformanceMonitor.Common.csproj | 4 + 3 files changed, 255 insertions(+), 2 deletions(-) diff --git a/Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs b/Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs index acf8537955..1196f74266 100644 --- a/Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs +++ b/Darling/Darling.Tests/DarlingMcpAlertToolsTests.cs @@ -12,6 +12,7 @@ using System.Linq; using System.Reflection; using System.Text.Json; +using System.Text.Json.Nodes; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.DependencyInjection; @@ -219,6 +220,173 @@ public void AlertSettingsSelect_ColumnCount_MatchesTheOrdinalsRead() Assert.Equal(columns - 1, ordinals.Max()); } + /// + /// The round-trip invariant, DERIVED rather than hand-pinned: the set of columns the reader SELECTs must + /// equal the set of columns update_alert_settings writes when it is handed get_alert_settings' + /// own payload. That is the round trip the tool's description tells a caller to perform — read the + /// settings, change one number, write them back — asserted as one set comparison rather than as a list of + /// keys somebody has to remember to extend. + /// + /// #2417 found the drift running in BOTH directions at once, which is why the assertion is an + /// equality and not a subset. Six columns were read out of the store on every call and emitted to nobody + /// (the AG four and the connection two) — those are missing from the write set. And one key WAS emitted + /// that the writer refused (blocking.wait_threshold_seconds), so feeding a read payload back + /// failed with "Unknown field", naming a field the caller never chose to send — that shows up as a + /// non-null parse error instead of a column set. The third direction, a key emitted for a column nobody + /// reads, no hand-maintained list would have noticed at all. + /// + /// There are NO exemptions, deliberately. Every column on this row is a knob the Viewer's Settings + /// window already writes, so leaving one readable-but-not-writable just moves an operator to a hand-typed + /// UPDATE — and it would need an exemption entry, which is the same hand-maintained fact that let six + /// columns go missing in the first place. If a genuinely read-only column ever arrives, exempt it BY NAME + /// here with its reason; do not loosen the equality. + /// + [Fact] + public void EveryColumnRead_IsEmittedByThePayload_AndAcceptedByTheWriter() + { + var (columns, error) = ParseAsPartialUpdate(SerializedSettingsPayload(SampleSettingsRow())); + + /* Every key the payload emits is accepted. The parse stops at the FIRST rejection, so a non-null + error here names the exact key update_alert_settings would refuse from its own read. */ + Assert.Null(error); + + /* Each accepted key claimed its own column — two keys sharing one would make whichever lost the + parse order a silent no-op, with the caller told both were updated. */ + Assert.Equal(columns.Count, columns.Distinct(StringComparer.Ordinal).Count()); + + Assert.Equal( + SelectedAlertSettingsColumns().OrderBy(c => c, StringComparer.Ordinal).ToArray(), + columns.OrderBy(c => c, StringComparer.Ordinal).ToArray()); + } + + /// + /// The write bounds for everything #2417 made writable, against DarlingAlertSettings' clamps — + /// the same parity holds for the file-growth + /// knobs, and for the same reason: a bound that differs lets the tool ACCEPT a value the engine then + /// silently rewrites on read, which presents as the setting not sticking. Nothing says no. + /// + /// The two booleans are asserted to reach the engine UNCLAMPED. That is the case that would + /// otherwise go unnoticed — a range added to one of them later needs a matching gate here, and this is + /// what makes the day it appears loud. + /// + /// Zero is IN range on all four numerics because 0 is a shipped configuration on each: no + /// connection re-fire, no AG lag gate, no redo-queue gate (ag_redo_queue_alert_kb ships at 0 + /// precisely because a healthy queue size is workload-specific), no second blocking gate. A floor of 1 + /// would make the shipped row unwritable through the tool that reports it. + /// + [Fact] + public void AgConnectionAndBlockingWaitWriteBounds_MatchTheEngineClamps() + { + var tools = ReadRepoFile(System.IO.Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "Mcp", "DarlingMcpAlertTools.cs")); + var settings = ReadRepoFile(System.IO.Path.Combine( + "Darling", "PerformanceMonitor.Darling.Service", "DarlingAlertSettings.cs")); + + /* engine */ + Assert.Contains("NotifyConnectionDownAtStartup => _config.Alerts.NotifyConnectionDownAtStartup;", settings, StringComparison.Ordinal); + Assert.Contains("_config.Alerts.ConnectionRefireMinutes, 0, 1440", settings, StringComparison.Ordinal); + Assert.Contains("NotifyAgHealth => _config.Alerts.NotifyAgHealth;", settings, StringComparison.Ordinal); + Assert.Contains("_config.Alerts.AgLagAlertSeconds, 0, 86400", settings, StringComparison.Ordinal); + Assert.Contains("_config.Alerts.AgRedoQueueAlertKb, 0L, 1073741824L", settings, StringComparison.Ordinal); + Assert.Contains("_config.Alerts.AgDisconnectRefireMinutes, 0, 1440", settings, StringComparison.Ordinal); + Assert.Contains("Math.Max(0, _config.Alerts.BlockingWaitSecondsThreshold)", settings, StringComparison.Ordinal); + + /* tool: the same numbers, and no gate at all on the two booleans */ + Assert.Contains("AddBool(\"notify_connection_down_at_startup\"", tools, StringComparison.Ordinal); + Assert.Contains("\"connection_refire_minutes\", 0, 1440", tools, StringComparison.Ordinal); + Assert.Contains("AddBool(\"notify_ag_health\"", tools, StringComparison.Ordinal); + Assert.Contains("\"ag.lag_threshold_seconds\", 0, 86400", tools, StringComparison.Ordinal); + Assert.Contains("\"ag.redo_queue_threshold_kb\", 0L, 1073741824L", tools, StringComparison.Ordinal); + Assert.Contains("\"ag.disconnect_refire_minutes\", 0, 1440", tools, StringComparison.Ordinal); + Assert.Contains("\"blocking.wait_threshold_seconds\", 0, int.MaxValue", tools, StringComparison.Ordinal); + } + + /// The reader's SELECT list, split from the SHIPPED constant the same way + /// counts it — so it cannot drift + /// from what the reader actually asks the store for. + private static IReadOnlyList SelectedAlertSettingsColumns() + { + var sql = Reader.AlertSettingsSelectSql; + var select = sql[(sql.IndexOf("SELECT", StringComparison.Ordinal) + 6).. + sql.IndexOf("FROM config_alert_settings", StringComparison.Ordinal)]; + return select.Split(',', StringSplitOptions.RemoveEmptyEntries) + .Select(c => c.Trim()) + .Where(c => c.Length > 0) + .ToList(); + } + + /// get_alert_settings' payload for one row, serialized through the SAME options the tool + /// serializes with and re-parsed. Runtime rather than source-parsing for the reason Lite's + /// McpAlertSettingsKeyTests gives: the C# identifier is not automatically the wire key, so only + /// serializing proves what a client receives — and therefore what it would hand back. + private static JsonObject SerializedSettingsPayload(Reader.AlertSettingsReadRow row) + { + var build = typeof(DarlingMcpAlertTools).GetMethod( + "BuildAlertSettingsPayload", BindingFlags.NonPublic | BindingFlags.Static)!; + var payload = build.Invoke(null, new object[] { row })!; + var json = JsonSerializer.Serialize(payload, payload.GetType(), McpHelpers.JsonOptions); + return (JsonObject)JsonNode.Parse(json)!; + } + + /// Runs a body through the tool's REAL partial-update parser and reports the columns it would + /// write plus the first validation error, if any. + private static (IReadOnlyList Columns, string? Error) ParseAsPartialUpdate(JsonObject body) + { + var build = typeof(DarlingMcpAlertTools).GetMethod( + "BuildAlertSettingsUpdate", BindingFlags.NonPublic | BindingFlags.Static)!; + var result = build.Invoke(null, new object[] { body })!; + var type = result.GetType(); + var updates = (IEnumerable<(string Column, NpgsqlParameter Param)>)type.GetField("Item1")!.GetValue(result)!; + return (updates.Select(u => u.Column).ToList(), (string?)type.GetField("Item2")!.GetValue(result)); + } + + /// A plausible settings row whose every value sits INSIDE the writer's bounds, so the invariant + /// above fails on a missing or unaccepted KEY rather than on a value. Named arguments deliberately: a new + /// column makes this stop compiling until someone supplies it, which is the moment to decide whether the + /// payload and the writer learn about it too. (Ordinal safety of the SELECT itself is + /// 's job, not this one's.) + private static Reader.AlertSettingsReadRow SampleSettingsRow() => new( + Enabled: true, + CpuEnabled: true, CpuThresholdPercent: 80, CpuMode: "sql", + BlockingEnabled: true, BlockingCountThreshold: 5, + DeadlockEnabled: true, DeadlockCountThreshold: 3, + PoisonWaitEnabled: true, PoisonWaitThresholdMs: 1000, + LongRunningQueryEnabled: true, LongRunningQueryThresholdMinutes: 5, + TempDbSpaceEnabled: true, TempDbSpaceThresholdPercent: 80, + LowDiskEnabled: true, LowDiskThresholdPercent: 10, LowDiskThresholdGb: 20, + LongRunningJobEnabled: true, LongRunningJobMultiplier: 3, + FailedJobEnabled: true, FailedJobLookbackMinutes: 60, + CooldownMinutes: 15, + ExcludedDatabases: new[] { "tempdb" }, + AnalysisEnabled: true, AnalysisIntervalMinutes: 60, + AnalysisNotificationsEnabled: true, AnalysisNotifySeverity: 1.0, + DeliveryMode: "Summary", PerEventMax: 10, + LongRunningQueryMaxResults: 25, + LongRunningQueryExcludeSpServerDiagnostics: true, + LongRunningQueryExcludeWaitFor: true, + LongRunningQueryExcludeBackups: true, + LongRunningQueryExcludeMiscWaits: true, + LongRunningQueryExcludeCdc: true, + NotifyConnectionChanges: true, + NotifyConnectionDownAtStartup: true, + ConnectionRefireMinutes: 30, + NotifyAgHealth: true, + AgLagAlertSeconds: 300, + AgRedoQueueAlertKb: 1048576L, + AgDisconnectRefireMinutes: 30, + BlockingWaitSecondsThreshold: 60, + PvsEnabled: true, PvsThresholdPercent: 40, PvsFloorGb: 1, + DatabaseStateEnabled: true, + SelfDiskFreeWarnPercent: 15, + CollectionStaleMinutes: 30, + CollectionFailureThreshold: 3, + DiskCriticalFreePercent: 5, + DiskCriticalFreeGb: 10, + AnalysisNotifyCooldownMinutes: 360, + StoreJobCadenceWarnPercent: 80, + FileGrowthEnabled: true, FileGrowthRiseMb: 1024, FileGrowthVolumePercent: 10, + FileGrowthLookbackMinutes: 60); + private static string ReadRepoFile( string relative, [System.Runtime.CompilerServices.CallerFilePath] string thisFile = "") { diff --git a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.cs b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.cs index 3c6c3b51b5..5edc6322f0 100644 --- a/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.cs +++ b/Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.cs @@ -123,7 +123,7 @@ public static async Task GetAlertHistory( } } - [McpServerTool(Name = "get_alert_settings"), Description("Gets the current alert configuration the service is using: which alerts are enabled and their thresholds (CPU, blocking, deadlocks, poison waits, long-running queries/jobs, tempdb, low disk, failed jobs, database state), the cooldown, excluded databases, the deadlock/blocking delivery mode, and the scheduled-analysis cadence. This is the single global settings row the service hot-swaps in. SMTP/webhook delivery credentials are managed separately and are not reported here.")] + [McpServerTool(Name = "get_alert_settings"), Description("Gets the current alert configuration the service is using: which alerts are enabled and their thresholds (CPU, blocking, deadlocks, poison waits, long-running queries/jobs, tempdb, low disk, failed jobs, database state, Availability Group health, connection loss), the cooldown, excluded databases, the deadlock/blocking delivery mode, and the scheduled-analysis cadence. This is the single global settings row the service hot-swaps in. SMTP/webhook delivery credentials are managed separately and are not reported here.")] public static async Task GetAlertSettings( NpgsqlDataSource postgres) { @@ -144,11 +144,26 @@ public static async Task GetAlertSettings( } /// The nested JSON shape get_alert_settings returns AND update_alert_settings echoes back — the same - /// field names update_alert_settings accepts on the way in, so a read → modify → write round-trips. + /// field names update_alert_settings accepts on the way in, so a read → modify → write round-trips. + /// + /// That last clause is an INVARIANT, not a habit: #2417 found it broken in both directions at once + /// (six columns read and emitted to nobody, one key emitted the writer refused). It is now asserted as a + /// set equality between this payload's keys and the columns AlertSettingsSelectSql reads — + /// EveryColumnRead_IsEmittedByThePayload_AndAcceptedByTheWriter. Adding a key here without the + /// matching arm in (or the reverse) fails that test rather than + /// shipping. private static object BuildAlertSettingsPayload(DarlingAlertReader.AlertSettingsReadRow s) => new { alerts_enabled = s.Enabled, notify_connection_changes = s.NotifyConnectionChanges, + /* #2417: the connection family's two sub-settings, kept TOP-LEVEL beside their master switch + rather than folded into a `connection` group. The master shipped as a top-level key and Lite + emits it there too, so a group could only ever hold two of the three -- splitting one family + across two levels of the document is worse for a reader than two extra top-level keys. The + spellings are the store's own column names, which are also the keys Lite already reads out of + settings.json (App.LoadAlertSettings), so Lite's payload can adopt them verbatim. */ + notify_connection_down_at_startup = s.NotifyConnectionDownAtStartup, + connection_refire_minutes = s.ConnectionRefireMinutes, cpu = new { enabled = s.CpuEnabled, threshold_percent = s.CpuThresholdPercent, mode = s.CpuMode }, blocking = new { @@ -203,6 +218,22 @@ be enabled only by UPDATEing config_alert_settings by hand. Reported by @gotqn. long_running_job = new { enabled = s.LongRunningJobEnabled, multiplier = s.LongRunningJobMultiplier }, failed_job = new { enabled = s.FailedJobEnabled, lookback_minutes = s.FailedJobLookbackMinutes }, database_state = new { enabled = s.DatabaseStateEnabled }, + /* #2417: the AG family, read out of the store since V35/V37 and emitted to nobody until now -- so + an agent asked why nothing alerted when a replica fell behind could not see the lag threshold, + could not see whether AG notification was on at all, and had no way to tell "configured not to + alert" from "failed to alert". Grouped like every other alert family, and the master switch is + named `enabled` for the same reason cpu/blocking/pvs/database_state are: inside this payload + `enabled` always means "this alert family is on", and notify_ag_health IS that switch rather + than a second opt-in behind one. The thresholds take the house _threshold_ spelling + (blocking.wait_threshold_seconds, poison_wait.threshold_ms, low_disk.threshold_gb) instead of + transliterating the columns' older _alert_ suffix, which appears nowhere else on the wire. */ + ag = new + { + enabled = s.NotifyAgHealth, + lag_threshold_seconds = s.AgLagAlertSeconds, + redo_queue_threshold_kb = s.AgRedoQueueAlertKb, + disconnect_refire_minutes = s.AgDisconnectRefireMinutes + }, cooldown_minutes = s.CooldownMinutes, excluded_databases = s.ExcludedDatabases, delivery = new { mode = s.DeliveryMode, per_event_max = s.PerEventMax }, @@ -471,6 +502,26 @@ void AddInt(string column, JsonNode? node, string field, int min, int max) } } + /* ag_redo_queue_alert_kb is the one bigint on this row, and DarlingAlertSettings clamps it in + long arithmetic. Binding it through AddInt would work today only because the ceiling happens to + fit in an int; typing it here means a raised ceiling stays writable instead of silently + rejecting every value above int.MaxValue as "not an integer". */ + void AddLong(string column, JsonNode? node, string field, long min, long max) + { + if (error != null) return; + if (node is JsonValue v && v.TryGetValue(out var l)) + { + if (l < min || l > max) + error = $"'{field}' must be an integer between {min} and {max}."; + else + updates.Add((column, new NpgsqlParameter { TypedValue = l })); + } + else + { + error = $"'{field}' must be an integer between {min} and {max}."; + } + } + void AddDouble(string column, JsonNode? node, string field, double min, double max) { if (error != null) return; @@ -556,6 +607,11 @@ void Group(JsonNode? node, string group, Action handleKey) { case "alerts_enabled": AddBool("enabled", prop.Value, "alerts_enabled"); break; case "notify_connection_changes": AddBool("notify_connection_changes", prop.Value, "notify_connection_changes"); break; + /* #2417: bounds mirror DarlingAlertSettings' clamps EXACTLY -- Clamp(0, 1440) on the + refire, nothing to clamp on the at-startup opt-in. Zero is IN range because 0 is the + shipped configuration (one alert per outage, no re-fire), not an invalid one. */ + case "notify_connection_down_at_startup": AddBool("notify_connection_down_at_startup", prop.Value, "notify_connection_down_at_startup"); break; + case "connection_refire_minutes": AddInt("connection_refire_minutes", prop.Value, "connection_refire_minutes", 0, 1440); break; case "cooldown_minutes": AddInt("cooldown_minutes", prop.Value, "cooldown_minutes", 1, 120); break; case "excluded_databases": AddStringArray("excluded_databases", prop.Value, "excluded_databases"); break; @@ -579,6 +635,13 @@ void Group(JsonNode? node, string group, Action handleKey) { case "enabled": AddBool("blocking_enabled", n, "blocking.enabled"); break; case "count_threshold": AddInt("blocking_count_threshold", n, "blocking.count_threshold", 1, int.MaxValue); break; + /* #2417: get_alert_settings has emitted this key since #1839 and the writer + never took it, so handing a whole read payload back -- the round trip this + tool's own description tells the caller to perform -- was rejected with + "Unknown field 'blocking.wait_threshold_seconds'", naming a field the caller + did not choose to send. The bound is the engine's Math.Max(0, ...), so 0 + keeps disabling the second gate rather than becoming invalid. */ + case "wait_threshold_seconds": AddInt("blocking_wait_seconds_threshold", n, "blocking.wait_threshold_seconds", 0, int.MaxValue); break; default: error = $"Unknown field 'blocking.{k}'."; break; } }); @@ -738,6 +801,24 @@ which is why the rise floor is 0 and not 1. */ }); break; + /* #2417: bounds mirror DarlingAlertSettings' clamps EXACTLY -- Clamp(lag, 0, 86400), + Clamp(redo, 0L, 1073741824L), Clamp(refire, 0, 1440). Zero is IN range on all three + and means OFF for that gate (the redo queue SHIPS at 0, because a healthy queue size + is workload-specific), so a floor of 1 would remove the shipped configuration. */ + case "ag": + Group(prop.Value, "ag", (k, n) => + { + switch (k) + { + case "enabled": AddBool("notify_ag_health", n, "ag.enabled"); break; + case "lag_threshold_seconds": AddInt("ag_lag_alert_seconds", n, "ag.lag_threshold_seconds", 0, 86400); break; + case "redo_queue_threshold_kb": AddLong("ag_redo_queue_alert_kb", n, "ag.redo_queue_threshold_kb", 0L, 1073741824L); break; + case "disconnect_refire_minutes": AddInt("ag_disconnect_refire_minutes", n, "ag.disconnect_refire_minutes", 0, 1440); break; + default: error = $"Unknown field 'ag.{k}'."; break; + } + }); + break; + case "delivery": Group(prop.Value, "delivery", (k, n) => { diff --git a/PerformanceMonitor.Common/PerformanceMonitor.Common.csproj b/PerformanceMonitor.Common/PerformanceMonitor.Common.csproj index fb06642be5..612b4bcd0b 100644 --- a/PerformanceMonitor.Common/PerformanceMonitor.Common.csproj +++ b/PerformanceMonitor.Common/PerformanceMonitor.Common.csproj @@ -36,5 +36,9 @@ + + From 01612813dbe1326345ffc1eb4a26826c232181de Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 13:51:35 +0100 Subject: [PATCH 2/3] Give Lite the AG and connection groups Darling now emits #2410 landed on dev between this branch's base and its first CI run, and it changed the rules: GetAlertSettings_ReportsEveryGroupDarlingDoes_SpelledDarlingsWay parses Darling's BuildAlertSettingsPayload and requires Lite's payload to carry every group and key it finds. So Darling's half of #2417 could not merge alone after all -- the Lite build went red naming exactly the three additions, missing group 'notify_connection_down_at_startup', 'connection_refire_minutes' and 'ag'. That test is doing precisely what it was written to do, and the answer is to make Lite follow now rather than to weaken it. Lite needs no new plumbing for six of the seven. App.NotifyConnectionDownAtStartup and App.ConnectionRefireMinutes have driven the connection-alert path since #1659, and App.NotifyAgHealth, App.AgLagAlertSeconds and App.AgRedoQueueAlertKb have driven all four AG conditions since #1726. Only the MCP surface was narrow, which is the same shape of gap #2394 fixed for the other nine groups. The seventh is a real asymmetry rather than an oversight, so it is exempted by name instead of faked. Darling's ag_disconnect_refire_minutes is #1696 / store V37, and Lite has no AG disconnect re-fire at ALL -- no static, no settings.json key, and no edge state that could re-announce a still-disconnected replica, because #1726 mirrored three of those four knobs. Emitting it as a constant 0 would tell an agent it can tune something Lite cannot, which is the reasoning the file already applies to self_alerts, here at member granularity instead of group. LiteOmittedMembers holds the one entry, the comparison skips exactly that key, and a new test asserts BOTH halves still hold -- that Darling still emits it, so the exemption cannot become dead weight hiding a Darling regression, and that Lite still does not, so it cannot mask a key that has since arrived. The set is pinned exactly, so a second exemption has to be justified in that test the way a second Lite-only group has to be justified in the smtp assertion. Lite.Tests cannot run on macOS, so the parity test's parser was ported to Python and validated against the test's OWN self-checks before being trusted -- the 15-40 group count, cpu/analysis/self_alerts present, smtp absent, and cpu's members exactly enabled/threshold_percent/mode. Run against dev's Lite payload it reproduces CI's failure message byte for byte; against this branch's it passes; and with the exemption removed it fails on ag.disconnect_refire_minutes and nothing else, which is what shows the exemption is both load-bearing and minimal. Darling's own round-trip invariant re-ran green after the merge. The gap itself is worth closing and is not closed here: a replica disconnected for a week is announced once on Lite where Darling can re-announce it. That is #1696's Lite half, a feature with its own static, settings row, box and edge state, and it does not belong in a payload-symmetry change. Co-Authored-By: Claude Opus 5 --- Lite.Tests/McpAlertSettingsKeyTests.cs | 42 +++++++++++++++++++++++++- Lite/Mcp/McpAlertTools.cs | 28 ++++++++++++++++- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/Lite.Tests/McpAlertSettingsKeyTests.cs b/Lite.Tests/McpAlertSettingsKeyTests.cs index ac439868c0..ace9e8df95 100644 --- a/Lite.Tests/McpAlertSettingsKeyTests.cs +++ b/Lite.Tests/McpAlertSettingsKeyTests.cs @@ -136,6 +136,18 @@ private static string FindRepoFile(string relativePath) /// decision in both places that reference it. private const string SelfAlertsGroup = "self_alerts"; + /// #2417: the MEMBER-level counterpart to — Darling keys Lite + /// genuinely has no equivalent for, exempted BY NAME so the hole is a decision someone justified here + /// rather than a loosened assertion. Each entry is paid for by a test asserting the omission is still + /// real, so an exemption cannot outlive its reason. + /// + /// ag.disconnect_refire_minutes is Darling's #1696 / store-V37 knob. Lite's AG work + /// (#1726) mirrored three of those four knobs and has no re-fire at all — no static, no settings.json + /// key, no edge state that could re-announce a still-disconnected replica. Emitting it as a constant 0 + /// would tell an agent it can tune something Lite cannot, which is the same reasoning that omits + /// self_alerts whole. + private static readonly string[] LiteOmittedMembers = { "ag.disconnect_refire_minutes" }; + /// /// Darling's BuildAlertSettingsPayload shape read out of Darling's SOURCE — each top-level key in /// document order with its nested keys (an empty list for a scalar like cooldown_minutes). Derived @@ -256,7 +268,10 @@ public void GetAlertSettings_ReportsEveryGroupDarlingDoes_SpelledDarlingsWay() if (darlingKeys.Count == 0) continue; var liteKeys = KeysOf(element); - problems.AddRange(darlingKeys.Except(liteKeys).Select(k => $"missing '{group}.{k}'")); + problems.AddRange(darlingKeys + .Where(k => !LiteOmittedMembers.Contains($"{group}.{k}", StringComparer.Ordinal)) + .Except(liteKeys) + .Select(k => $"missing '{group}.{k}'")); problems.AddRange(liteKeys.Except(darlingKeys).Select(k => $"'{group}.{k}' is Lite-only")); } @@ -272,6 +287,31 @@ added without this test being the place someone justifies it. */ KeysOf(root).Except(darling.Select(g => g.Key)).ToArray()); } + /// + /// The one MEMBER Lite deliberately does not report, asserted for the same reason its group-level + /// sibling below is: so the hole reads as a decision rather than the oversight it would otherwise look + /// like — and so the exemption cannot quietly outlive its reason. Both halves are asserted. Darling + /// must still emit it (otherwise the exemption is dead weight hiding a Darling regression), and Lite + /// must still not (otherwise the exemption is masking a key that has since arrived and could now be + /// compared). + /// + /// The gap itself is real and worth closing: Lite has no AG disconnect re-fire, so a replica + /// disconnected for a week is announced exactly once here where Darling can re-announce it. That is + /// #1696's Lite half, and it is a feature rather than a payload key — which is why this PR reports the + /// three knobs Lite really has instead of inventing a fourth. + /// + [Fact] + public void GetAlertSettings_OmitsAgDisconnectRefire_WhichLiteHasNoEquivalentFor() + { + var ag = DarlingPayloadShape().Single(g => g.Key == "ag").Value; + Assert.Contains("disconnect_refire_minutes", ag); + Assert.DoesNotContain("disconnect_refire_minutes", KeysOf(Settings().GetProperty("ag"))); + + /* Pinned as an exact set, so a second member-level exemption cannot be added without this test + being the place someone justifies it — the same guard the smtp assertion applies to groups. */ + Assert.Equal(new[] { "ag.disconnect_refire_minutes" }, LiteOmittedMembers); + } + /// /// The one group Lite deliberately does NOT report, asserted so the hole reads as a decision rather than /// the oversight it would otherwise look like. AppAlertEngineSettings returns shipped constants for diff --git a/Lite/Mcp/McpAlertTools.cs b/Lite/Mcp/McpAlertTools.cs index 317d7ae707..6f2eb16dcf 100644 --- a/Lite/Mcp/McpAlertTools.cs +++ b/Lite/Mcp/McpAlertTools.cs @@ -76,7 +76,7 @@ public static async Task GetAlertHistory( } } - [McpServerTool(Name = "get_alert_settings"), Description("Gets the current alert configuration this instance is running on: which alerts are enabled and their thresholds (CPU, blocking, deadlocks, poison waits, long-running queries and jobs, tempdb space, low disk, PVS, file growth, failed jobs, database state), the cooldown, the excluded databases, the deadlock/blocking delivery mode, the scheduled-analysis cadence, and the SMTP email configuration. The same nested shape Darling's get_alert_settings returns, minus its self_alerts group (the headless service's own store-volume and collection-health thresholds, which a single-instance Lite install has no equivalent for) and plus smtp, which Lite delivers itself. Read-only: Lite has no update_alert_settings, so these change in the Settings window.")] + [McpServerTool(Name = "get_alert_settings"), Description("Gets the current alert configuration this instance is running on: which alerts are enabled and their thresholds (CPU, blocking, deadlocks, poison waits, long-running queries and jobs, tempdb space, low disk, PVS, file growth, failed jobs, database state, Availability Group health, connection loss), the cooldown, the excluded databases, the deadlock/blocking delivery mode, the scheduled-analysis cadence, and the SMTP email configuration. The same nested shape Darling's get_alert_settings returns, minus its self_alerts group (the headless service's own store-volume and collection-health thresholds, which a single-instance Lite install has no equivalent for) and plus smtp, which Lite delivers itself. Read-only: Lite has no update_alert_settings, so these change in the Settings window.")] public static Task GetAlertSettings() { try @@ -88,6 +88,13 @@ public static Task GetAlertSettings() sub-object's own toggle), so the old Lite name was not merely different — it collided. */ alerts_enabled = App.AlertsEnabled, notify_connection_changes = App.NotifyConnectionChanges, + /* #2417: the connection family's two sub-settings. Darling keeps these TOP-LEVEL beside + their master switch rather than in a `connection` group, because the master shipped as a + top-level key and a group could only ever have held two of the three; the spellings are + its column names, which are also the keys Lite's own settings.json already uses. Both + statics have been wired through to the connection-alert path since #1659. */ + notify_connection_down_at_startup = App.NotifyConnectionDownAtStartup, + connection_refire_minutes = App.ConnectionRefireMinutes, cpu = new { enabled = App.AlertCpuEnabled, @@ -191,6 +198,25 @@ Reporting constants under names that read as knobs would tell an agent it can tu lookback_minutes = App.AlertFailedJobLookbackMinutes }, database_state = new { enabled = App.AlertDatabaseStateEnabled }, + /* #2417: the Availability Group family, which Darling emitted to nobody until now and + Lite therefore could not mirror. Lite has evaluated all four AG conditions since #1726 + off these same statics, so this is surface, not new alerting. Group and key spellings + are Darling's verbatim, including `enabled` for what the store calls notify_ag_health — + inside this payload `enabled` always means "this alert family is on". + + Darling reports a FOURTH member here, disconnect_refire_minutes (#1696, store V37), and + it is deliberately absent: Lite has no AG disconnect re-fire at ALL — no static, no + settings.json key, and no edge state that could re-announce — because #1726 mirrored + three of those four knobs. Emitting a constant 0 would tell an agent it can tune + something Lite cannot, which is exactly why self_alerts is omitted whole above. The + omission is pinned by name in McpAlertSettingsKeyTests so it cannot outlive its + reason. */ + ag = new + { + enabled = App.NotifyAgHealth, + lag_threshold_seconds = App.AgLagAlertSeconds, + redo_queue_threshold_kb = App.AgRedoQueueAlertKb + }, cooldown_minutes = App.AlertCooldownMinutes, excluded_databases = App.AlertExcludedDatabases, delivery = new From 03283ef9f3c8148c20ec5e497048a1bdffdca449 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 13:52:51 +0100 Subject: [PATCH 3/3] Point the AG re-fire exemption at the issue that tracks it The omission is only honest if the reader can find out what is being omitted and whether anyone intends to fix it, so the exemption in Lite's payload and in the parity test both name #2426 rather than describing the gap and leaving it nowhere. Co-Authored-By: Claude Opus 5 --- Lite.Tests/McpAlertSettingsKeyTests.cs | 5 +++-- Lite/Mcp/McpAlertTools.cs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Lite.Tests/McpAlertSettingsKeyTests.cs b/Lite.Tests/McpAlertSettingsKeyTests.cs index ace9e8df95..80712f4924 100644 --- a/Lite.Tests/McpAlertSettingsKeyTests.cs +++ b/Lite.Tests/McpAlertSettingsKeyTests.cs @@ -145,7 +145,8 @@ private static string FindRepoFile(string relativePath) /// (#1726) mirrored three of those four knobs and has no re-fire at all — no static, no settings.json /// key, no edge state that could re-announce a still-disconnected replica. Emitting it as a constant 0 /// would tell an agent it can tune something Lite cannot, which is the same reasoning that omits - /// self_alerts whole. + /// self_alerts whole. Filed as #2426 — the exemption is the honest report of a gap, not the fix for + /// it. private static readonly string[] LiteOmittedMembers = { "ag.disconnect_refire_minutes" }; /// @@ -298,7 +299,7 @@ added without this test being the place someone justifies it. */ /// The gap itself is real and worth closing: Lite has no AG disconnect re-fire, so a replica /// disconnected for a week is announced exactly once here where Darling can re-announce it. That is /// #1696's Lite half, and it is a feature rather than a payload key — which is why this PR reports the - /// three knobs Lite really has instead of inventing a fourth. + /// three knobs Lite really has instead of inventing a fourth. Filed as #2426. /// [Fact] public void GetAlertSettings_OmitsAgDisconnectRefire_WhichLiteHasNoEquivalentFor() diff --git a/Lite/Mcp/McpAlertTools.cs b/Lite/Mcp/McpAlertTools.cs index 6f2eb16dcf..8e8c272ecf 100644 --- a/Lite/Mcp/McpAlertTools.cs +++ b/Lite/Mcp/McpAlertTools.cs @@ -210,7 +210,7 @@ inside this payload `enabled` always means "this alert family is on". three of those four knobs. Emitting a constant 0 would tell an agent it can tune something Lite cannot, which is exactly why self_alerts is omitted whole above. The omission is pinned by name in McpAlertSettingsKeyTests so it cannot outlive its - reason. */ + reason, and the gap itself is filed as #2426. */ ag = new { enabled = App.NotifyAgHealth,