From 97045688d2bcec3f478ca22fde68f9cec5d6b089 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:56:14 -0400 Subject: [PATCH 1/4] Render only the incidents outside their own cooldown window (#3313) IncidentCooldown decides WHETHER an alert posts and, per #1154, posts when any one of its fingerprints is outside its own window. Nothing decided WHICH incidents the resulting card contained, so the channel builders rendered every entry in AlertContext.Incidents -- including ones still inside their own window that had been delivered minutes earlier. One fresh fingerprint dragged every co-resident stale one back into the channel with it. Decision now reports the fingerprints that were outside their own window, and IncidentDeliveryFilter narrows the rendered context to them, dropping each suppressed incident's own detail item and appending a one-line footer stating how many were held back. Applied once per send path -- the webhook fan-out and the email branch each filter from their own decision, because the two channels hold separate cooldown key spaces. Returns a copy, so the config_alert_log row the caller writes afterwards still carries every incident. --- .../AlertDeliveryChannelTests.cs | 335 ------------- .../Darling.Tests/CapturingAlertEndpoints.cs | 362 ++++++++++++++ .../IncidentDeliveryFilterTests.cs | 450 ++++++++++++++++++ Lite.Tests/AlertIncidentRenderTests.cs | 39 ++ Lite.Tests/IncidentCooldownTests.cs | 64 +++ .../AlertIncidentRenderer.cs | 10 +- .../EmailSendCore.cs | 19 +- .../IncidentCooldown.cs | 39 +- .../IncidentDeliveryFilter.cs | 201 ++++++++ .../WebhookAlertService.cs | 31 +- 10 files changed, 1195 insertions(+), 355 deletions(-) create mode 100644 Darling/Darling.Tests/CapturingAlertEndpoints.cs create mode 100644 Darling/Darling.Tests/IncidentDeliveryFilterTests.cs create mode 100644 PerformanceMonitor.Notifications/IncidentDeliveryFilter.cs diff --git a/Darling/Darling.Tests/AlertDeliveryChannelTests.cs b/Darling/Darling.Tests/AlertDeliveryChannelTests.cs index 1bb05d7ca5..6f05b54731 100644 --- a/Darling/Darling.Tests/AlertDeliveryChannelTests.cs +++ b/Darling/Darling.Tests/AlertDeliveryChannelTests.cs @@ -938,339 +938,4 @@ private static int CountOccurrences(string haystack, string needle) return count; } - - /// - /// A loopback endpoint that records the bodies posted to it. - /// rather than HttpListener on purpose: HttpListener wants a URL ACL on Windows, and the four - /// lines of HTTP a webhook POST needs are cheaper than that dependency. The same choice - /// NpgsqlRootCertificateValidationTests and DarlingStoreUpgradeTests already make. - /// - private sealed class CapturingWebhookEndpoint : IDisposable - { - private readonly TcpListener _listener; - private readonly List _bodies = new(); - private readonly CancellationTokenSource _stop = new(); - private readonly Task _accepting; - - public CapturingWebhookEndpoint() - { - _listener = new TcpListener(IPAddress.Loopback, 0); - _listener.Start(); - Url = $"http://127.0.0.1:{((IPEndPoint)_listener.LocalEndpoint).Port}/hook"; - _accepting = Task.Run(AcceptLoopAsync); - } - - public string Url { get; } - - /// - /// Safe to read without synchronization once the send has been awaited: each body is appended before - /// its response is written, and the sender awaits every response in turn. - /// - public IReadOnlyList Bodies => _bodies; - - private async Task AcceptLoopAsync() - { - try - { - while (!_stop.IsCancellationRequested) - { - using var client = await _listener.AcceptTcpClientAsync(_stop.Token); - using var stream = client.GetStream(); - _bodies.Add(await ReadRequestBodyAsync(stream)); - - var response = Encoding.ASCII.GetBytes( - "HTTP/1.1 200 OK\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"); - await stream.WriteAsync(response, _stop.Token); - await stream.FlushAsync(_stop.Token); - } - } - catch (OperationCanceledException) { /* Dispose */ } - catch (System.Net.Sockets.SocketException) { /* listener stopped */ } - catch (ObjectDisposedException) { /* listener stopped */ } - } - - /// Reads headers to the blank line, then exactly Content-Length bytes of body. - private static async Task ReadRequestBodyAsync(NetworkStream stream) - { - var buffer = new byte[16 * 1024]; - var received = new List(capacity: 16 * 1024); - int headerEnd; - - while ((headerEnd = IndexOfHeaderEnd(received)) < 0) - { - var read = await stream.ReadAsync(buffer); - if (read == 0) - { - return Encoding.UTF8.GetString(received.ToArray()); - } - - received.AddRange(new ArraySegment(buffer, 0, read)); - } - - var headers = Encoding.ASCII.GetString(received.ToArray(), 0, headerEnd); - var contentLength = ParseContentLength(headers); - var bodyStart = headerEnd + 4; - - while (received.Count - bodyStart < contentLength) - { - var read = await stream.ReadAsync(buffer); - if (read == 0) - { - break; - } - - received.AddRange(new ArraySegment(buffer, 0, read)); - } - - var body = received.ToArray(); - var available = Math.Min(contentLength, body.Length - bodyStart); - return Encoding.UTF8.GetString(body, bodyStart, Math.Max(0, available)); - } - - private static int IndexOfHeaderEnd(List bytes) - { - for (int i = 0; i + 3 < bytes.Count; i++) - { - if (bytes[i] == (byte)'\r' && bytes[i + 1] == (byte)'\n' && - bytes[i + 2] == (byte)'\r' && bytes[i + 3] == (byte)'\n') - { - return i; - } - } - - return -1; - } - - private static int ParseContentLength(string headers) - { - foreach (var line in headers.Split("\r\n", StringSplitOptions.RemoveEmptyEntries)) - { - if (line.StartsWith("Content-Length:", StringComparison.OrdinalIgnoreCase) && - int.TryParse(line.AsSpan("Content-Length:".Length).Trim(), out var length)) - { - return length; - } - } - - return 0; - } - - public void Dispose() - { - _stop.Cancel(); - _listener.Stop(); - try - { - _accepting.Wait(TimeSpan.FromSeconds(5)); - } - catch (AggregateException) { /* the cancellation above */ } - - _stop.Dispose(); - } - } - - /// - /// A loopback SMTP sink that records the DATA of each message posted to it, quoted-printable soft line - /// breaks removed so an assertion on a phrase cannot fail because MIME wrapped it at column 76. - /// Exists because email is the only channel with no interception point short of the protocol: - /// there is no builder-returns-the-payload seam between EmailSendCore and the wire. Without it a - /// mutation that drops the detail on the way to the email template alone passes every other pin — which - /// is exactly the defect #3296 reported, so leaving that one hop unpinned was not an option. - /// - private sealed class CapturingSmtpEndpoint : IDisposable - { - private readonly TcpListener _listener; - private readonly List _messages = new(); - private readonly CancellationTokenSource _stop = new(); - private readonly Task _accepting; - - public CapturingSmtpEndpoint() - { - _listener = new TcpListener(IPAddress.Loopback, 0); - _listener.Start(); - Port = ((IPEndPoint)_listener.LocalEndpoint).Port; - _accepting = Task.Run(AcceptLoopAsync); - } - - public int Port { get; } - - /// Safe to read once the send has been awaited; see the webhook endpoint's note. - public IReadOnlyList Messages => _messages; - - private async Task AcceptLoopAsync() - { - try - { - while (!_stop.IsCancellationRequested) - { - using var client = await _listener.AcceptTcpClientAsync(_stop.Token); - using var stream = client.GetStream(); - using var reader = new StreamReader(stream, Encoding.UTF8, false, 8192, leaveOpen: true); - using var writer = new StreamWriter(stream, new UTF8Encoding(false), 8192, leaveOpen: true) { AutoFlush = true, NewLine = "\r\n" }; - - await writer.WriteLineAsync("220 localhost ESMTP capture"); - - var data = new StringBuilder(); - var inData = false; - string? line; - while ((line = await reader.ReadLineAsync(_stop.Token)) is not null) - { - if (inData) - { - if (line == ".") - { - _messages.Add(DecodeMimeText(data.ToString())); - data.Clear(); - inData = false; - await writer.WriteLineAsync("250 OK queued"); - continue; - } - - /* Transparency: a body line starting with '.' arrives doubled. CRLF explicitly, - not AppendLine: MIME line endings are CRLF by the spec the parsing below - depends on, and Environment.NewLine is LF on the platform this is written on. */ - data.Append(line.StartsWith("..", StringComparison.Ordinal) ? line.Substring(1) : line).Append("\r\n"); - continue; - } - - if (line.StartsWith("EHLO", StringComparison.OrdinalIgnoreCase) || - line.StartsWith("HELO", StringComparison.OrdinalIgnoreCase)) - { - /* No extensions advertised, so the client never tries STARTTLS or AUTH. */ - await writer.WriteLineAsync("250 localhost"); - } - else if (line.StartsWith("DATA", StringComparison.OrdinalIgnoreCase)) - { - inData = true; - await writer.WriteLineAsync("354 End data with ."); - } - else if (line.StartsWith("QUIT", StringComparison.OrdinalIgnoreCase)) - { - await writer.WriteLineAsync("221 Bye"); - break; - } - else - { - await writer.WriteLineAsync("250 OK"); - } - } - } - } - catch (OperationCanceledException) { /* Dispose */ } - catch (IOException) { /* client went away */ } - catch (SocketException) { /* listener stopped */ } - catch (ObjectDisposedException) { /* listener stopped */ } - } - - /// - /// Returns the DECODED text of every MIME part, concatenated. Both transfer encodings this message - /// actually uses are handled — .NET picks base64 for the utf-8 plain-text alternate view and - /// quoted-printable for the us-ascii HTML one — because an assertion against the raw DATA would be - /// asserting against an encoding choice rather than against the delivered words. - /// - private static string DecodeMimeText(string raw) - { - var boundary = System.Text.RegularExpressions.Regex.Match(raw, @"boundary=(\S+)"); - if (!boundary.Success) - { - return DecodeQuotedPrintable(raw); - } - - var parts = raw.Split("--" + boundary.Groups[1].Value, StringSplitOptions.None); - var decoded = new StringBuilder(); - - foreach (var part in parts.Skip(1)) - { - var split = part.IndexOf("\r\n\r\n", StringComparison.Ordinal); - if (split < 0) - { - continue; - } - - var headers = part.Substring(0, split); - var body = part.Substring(split + 4); - - decoded.Append(headers.Contains("base64", StringComparison.OrdinalIgnoreCase) - ? Encoding.UTF8.GetString(Convert.FromBase64String( - new string(body.Where(c => !char.IsWhiteSpace(c)).ToArray()))) - : DecodeQuotedPrintable(body)).Append("\r\n"); - } - - return decoded.ToString(); - } - - /// Drops soft line breaks ("=" at end of line) and decodes "=XX" octets. - private static string DecodeQuotedPrintable(string raw) - { - var unfolded = raw.Replace("=\r\n", "", StringComparison.Ordinal).Replace("=\n", "", StringComparison.Ordinal); - var sb = new StringBuilder(unfolded.Length); - - for (int i = 0; i < unfolded.Length; i++) - { - if (unfolded[i] == '=' && i + 2 < unfolded.Length && - Uri.IsHexDigit(unfolded[i + 1]) && Uri.IsHexDigit(unfolded[i + 2])) - { - sb.Append((char)Convert.ToInt32(unfolded.Substring(i + 1, 2), 16)); - i += 2; - continue; - } - - sb.Append(unfolded[i]); - } - - return sb.ToString(); - } - - public void Dispose() - { - _stop.Cancel(); - _listener.Stop(); - try - { - _accepting.Wait(TimeSpan.FromSeconds(5)); - } - catch (AggregateException) { /* the cancellation above */ } - - _stop.Dispose(); - } - } - - /// - /// with the records kept, so a test can read what the row would - /// have held. Safe to read after the send has been awaited: SendFindingAlertAsync awaits its - /// own RecordAlertAsync. - /// - private sealed class CapturingHistoryStore : IAlertHistoryStore - { - public List Records { get; } = new(); - - public Task RecordAlertAsync(AlertHistoryRecord record) - { - Records.Add(record); - return Task.CompletedTask; - } - - public Task GetLastEmailSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => - Task.FromResult(null); - - public Task GetLastWebhookSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => - Task.FromResult(null); - - public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => - Task.FromResult(null); - } - - private sealed class DiscardingHistoryStore : IAlertHistoryStore - { - public Task RecordAlertAsync(AlertHistoryRecord record) => Task.CompletedTask; - - public Task GetLastEmailSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => - Task.FromResult(null); - - public Task GetLastWebhookSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => - Task.FromResult(null); - - public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => - Task.FromResult(null); - } } diff --git a/Darling/Darling.Tests/CapturingAlertEndpoints.cs b/Darling/Darling.Tests/CapturingAlertEndpoints.cs new file mode 100644 index 0000000000..2c1e0d3751 --- /dev/null +++ b/Darling/Darling.Tests/CapturingAlertEndpoints.cs @@ -0,0 +1,362 @@ +/* + * 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.Net; +using System.Net.Sockets; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using PerformanceMonitor.Notifications; + +namespace Darling.Tests; + +/// +/// The loopback channel endpoints and alert-history doubles the delivery suites drive. Shared rather than +/// nested per test class because the SMTP one in particular is the only interception point email has short +/// of the protocol (see its own remarks), and a second copy of a protocol responder is a second thing to +/// get subtly wrong. Used by (#3169/#3297/#3303) and +/// (#3313). +/// +/// +/// A loopback endpoint that records the bodies posted to it. +/// rather than HttpListener on purpose: HttpListener wants a URL ACL on Windows, and the four +/// lines of HTTP a webhook POST needs are cheaper than that dependency. The same choice +/// NpgsqlRootCertificateValidationTests and DarlingStoreUpgradeTests already make. +/// +internal sealed class CapturingWebhookEndpoint : IDisposable +{ + private readonly TcpListener _listener; + private readonly List _bodies = new(); + private readonly CancellationTokenSource _stop = new(); + private readonly Task _accepting; + + public CapturingWebhookEndpoint() + { + _listener = new TcpListener(IPAddress.Loopback, 0); + _listener.Start(); + Url = $"http://127.0.0.1:{((IPEndPoint)_listener.LocalEndpoint).Port}/hook"; + _accepting = Task.Run(AcceptLoopAsync); + } + + public string Url { get; } + + /// + /// Safe to read without synchronization once the send has been awaited: each body is appended before + /// its response is written, and the sender awaits every response in turn. + /// + public IReadOnlyList Bodies => _bodies; + + private async Task AcceptLoopAsync() + { + try + { + while (!_stop.IsCancellationRequested) + { + using var client = await _listener.AcceptTcpClientAsync(_stop.Token); + using var stream = client.GetStream(); + _bodies.Add(await ReadRequestBodyAsync(stream)); + + var response = Encoding.ASCII.GetBytes( + "HTTP/1.1 200 OK\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"); + await stream.WriteAsync(response, _stop.Token); + await stream.FlushAsync(_stop.Token); + } + } + catch (OperationCanceledException) { /* Dispose */ } + catch (System.Net.Sockets.SocketException) { /* listener stopped */ } + catch (ObjectDisposedException) { /* listener stopped */ } + } + + /// Reads headers to the blank line, then exactly Content-Length bytes of body. + private static async Task ReadRequestBodyAsync(NetworkStream stream) + { + var buffer = new byte[16 * 1024]; + var received = new List(capacity: 16 * 1024); + int headerEnd; + + while ((headerEnd = IndexOfHeaderEnd(received)) < 0) + { + var read = await stream.ReadAsync(buffer); + if (read == 0) + { + return Encoding.UTF8.GetString(received.ToArray()); + } + + received.AddRange(new ArraySegment(buffer, 0, read)); + } + + var headers = Encoding.ASCII.GetString(received.ToArray(), 0, headerEnd); + var contentLength = ParseContentLength(headers); + var bodyStart = headerEnd + 4; + + while (received.Count - bodyStart < contentLength) + { + var read = await stream.ReadAsync(buffer); + if (read == 0) + { + break; + } + + received.AddRange(new ArraySegment(buffer, 0, read)); + } + + var body = received.ToArray(); + var available = Math.Min(contentLength, body.Length - bodyStart); + return Encoding.UTF8.GetString(body, bodyStart, Math.Max(0, available)); + } + + private static int IndexOfHeaderEnd(List bytes) + { + for (int i = 0; i + 3 < bytes.Count; i++) + { + if (bytes[i] == (byte)'\r' && bytes[i + 1] == (byte)'\n' && + bytes[i + 2] == (byte)'\r' && bytes[i + 3] == (byte)'\n') + { + return i; + } + } + + return -1; + } + + private static int ParseContentLength(string headers) + { + foreach (var line in headers.Split("\r\n", StringSplitOptions.RemoveEmptyEntries)) + { + if (line.StartsWith("Content-Length:", StringComparison.OrdinalIgnoreCase) && + int.TryParse(line.AsSpan("Content-Length:".Length).Trim(), out var length)) + { + return length; + } + } + + return 0; + } + + public void Dispose() + { + _stop.Cancel(); + _listener.Stop(); + try + { + _accepting.Wait(TimeSpan.FromSeconds(5)); + } + catch (AggregateException) { /* the cancellation above */ } + + _stop.Dispose(); + } +} + +/// +/// A loopback SMTP sink that records the DATA of each message posted to it, quoted-printable soft line +/// breaks removed so an assertion on a phrase cannot fail because MIME wrapped it at column 76. +/// Exists because email is the only channel with no interception point short of the protocol: +/// there is no builder-returns-the-payload seam between EmailSendCore and the wire. Without it a +/// mutation that drops the detail on the way to the email template alone passes every other pin — which +/// is exactly the defect #3296 reported, so leaving that one hop unpinned was not an option. +/// +internal sealed class CapturingSmtpEndpoint : IDisposable +{ + private readonly TcpListener _listener; + private readonly List _messages = new(); + private readonly CancellationTokenSource _stop = new(); + private readonly Task _accepting; + + public CapturingSmtpEndpoint() + { + _listener = new TcpListener(IPAddress.Loopback, 0); + _listener.Start(); + Port = ((IPEndPoint)_listener.LocalEndpoint).Port; + _accepting = Task.Run(AcceptLoopAsync); + } + + public int Port { get; } + + /// Safe to read once the send has been awaited; see the webhook endpoint's note. + public IReadOnlyList Messages => _messages; + + private async Task AcceptLoopAsync() + { + try + { + while (!_stop.IsCancellationRequested) + { + using var client = await _listener.AcceptTcpClientAsync(_stop.Token); + using var stream = client.GetStream(); + using var reader = new StreamReader(stream, Encoding.UTF8, false, 8192, leaveOpen: true); + using var writer = new StreamWriter(stream, new UTF8Encoding(false), 8192, leaveOpen: true) { AutoFlush = true, NewLine = "\r\n" }; + + await writer.WriteLineAsync("220 localhost ESMTP capture"); + + var data = new StringBuilder(); + var inData = false; + string? line; + while ((line = await reader.ReadLineAsync(_stop.Token)) is not null) + { + if (inData) + { + if (line == ".") + { + _messages.Add(DecodeMimeText(data.ToString())); + data.Clear(); + inData = false; + await writer.WriteLineAsync("250 OK queued"); + continue; + } + + /* Transparency: a body line starting with '.' arrives doubled. CRLF explicitly, + not AppendLine: MIME line endings are CRLF by the spec the parsing below + depends on, and Environment.NewLine is LF on the platform this is written on. */ + data.Append(line.StartsWith("..", StringComparison.Ordinal) ? line.Substring(1) : line).Append("\r\n"); + continue; + } + + if (line.StartsWith("EHLO", StringComparison.OrdinalIgnoreCase) || + line.StartsWith("HELO", StringComparison.OrdinalIgnoreCase)) + { + /* No extensions advertised, so the client never tries STARTTLS or AUTH. */ + await writer.WriteLineAsync("250 localhost"); + } + else if (line.StartsWith("DATA", StringComparison.OrdinalIgnoreCase)) + { + inData = true; + await writer.WriteLineAsync("354 End data with ."); + } + else if (line.StartsWith("QUIT", StringComparison.OrdinalIgnoreCase)) + { + await writer.WriteLineAsync("221 Bye"); + break; + } + else + { + await writer.WriteLineAsync("250 OK"); + } + } + } + } + catch (OperationCanceledException) { /* Dispose */ } + catch (IOException) { /* client went away */ } + catch (SocketException) { /* listener stopped */ } + catch (ObjectDisposedException) { /* listener stopped */ } + } + + /// + /// Returns the DECODED text of every MIME part, concatenated. Both transfer encodings this message + /// actually uses are handled — .NET picks base64 for the utf-8 plain-text alternate view and + /// quoted-printable for the us-ascii HTML one — because an assertion against the raw DATA would be + /// asserting against an encoding choice rather than against the delivered words. + /// + private static string DecodeMimeText(string raw) + { + var boundary = System.Text.RegularExpressions.Regex.Match(raw, @"boundary=(\S+)"); + if (!boundary.Success) + { + return DecodeQuotedPrintable(raw); + } + + var parts = raw.Split("--" + boundary.Groups[1].Value, StringSplitOptions.None); + var decoded = new StringBuilder(); + + foreach (var part in parts.Skip(1)) + { + var split = part.IndexOf("\r\n\r\n", StringComparison.Ordinal); + if (split < 0) + { + continue; + } + + var headers = part.Substring(0, split); + var body = part.Substring(split + 4); + + decoded.Append(headers.Contains("base64", StringComparison.OrdinalIgnoreCase) + ? Encoding.UTF8.GetString(Convert.FromBase64String( + new string(body.Where(c => !char.IsWhiteSpace(c)).ToArray()))) + : DecodeQuotedPrintable(body)).Append("\r\n"); + } + + return decoded.ToString(); + } + + /// Drops soft line breaks ("=" at end of line) and decodes "=XX" octets. + private static string DecodeQuotedPrintable(string raw) + { + var unfolded = raw.Replace("=\r\n", "", StringComparison.Ordinal).Replace("=\n", "", StringComparison.Ordinal); + var sb = new StringBuilder(unfolded.Length); + + for (int i = 0; i < unfolded.Length; i++) + { + if (unfolded[i] == '=' && i + 2 < unfolded.Length && + Uri.IsHexDigit(unfolded[i + 1]) && Uri.IsHexDigit(unfolded[i + 2])) + { + sb.Append((char)Convert.ToInt32(unfolded.Substring(i + 1, 2), 16)); + i += 2; + continue; + } + + sb.Append(unfolded[i]); + } + + return sb.ToString(); + } + + public void Dispose() + { + _stop.Cancel(); + _listener.Stop(); + try + { + _accepting.Wait(TimeSpan.FromSeconds(5)); + } + catch (AggregateException) { /* the cancellation above */ } + + _stop.Dispose(); + } +} + +/// +/// with the records kept, so a test can read what the row would +/// have held. Safe to read after the send has been awaited: SendFindingAlertAsync awaits its +/// own RecordAlertAsync. +/// +internal sealed class CapturingHistoryStore : IAlertHistoryStore +{ + public List Records { get; } = new(); + + public Task RecordAlertAsync(AlertHistoryRecord record) + { + Records.Add(record); + return Task.CompletedTask; + } + + public Task GetLastEmailSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => + Task.FromResult(null); + + public Task GetLastWebhookSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => + Task.FromResult(null); + + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => + Task.FromResult(null); +} + +internal sealed class DiscardingHistoryStore : IAlertHistoryStore +{ + public Task RecordAlertAsync(AlertHistoryRecord record) => Task.CompletedTask; + + public Task GetLastEmailSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => + Task.FromResult(null); + + public Task GetLastWebhookSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => + Task.FromResult(null); + + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => + Task.FromResult(null); +} diff --git a/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs b/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs new file mode 100644 index 0000000000..6e94a0f692 --- /dev/null +++ b/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs @@ -0,0 +1,450 @@ +/* + * 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.Linq; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging.Abstractions; +using PerformanceMonitor.Alerting; +using PerformanceMonitor.Darling.Service; +using PerformanceMonitor.Notifications; +using Xunit; + +namespace Darling.Tests; + +/// +/// #3313: the per-fingerprint governed WHETHER a card posted and nothing +/// governed WHICH incidents it contained, so a single fresh fingerprint dragged every co-resident stale one +/// back into the channel with it. Measured on a 42-server store: a card delivered fingerprint +/// bf7e29b4 alone, then 41 minutes later re-delivered it as "Deadlock 2 of 2" riding on a new +/// fingerprint, with Total Occurrences: 1 proving it had not recurred. +/// +/// Both halves are pinned here because either alone is satisfiable by something wrong: "only the +/// fresh incident is delivered" is satisfied by dropping everything, and "the row carries both" is +/// satisfied by not filtering at all. +/// +/// The wire-level pins drive the whole Darling path — +/// through the shared send core, the fan-out and each channel's payload builder — and read the bytes that +/// left the process, for the reason #3297's pin in gives: the defect +/// is in what a builder is HANDED, and a builder-level assertion is green on a fan-out that hands it the +/// wrong thing. Lite runs the same PerformanceMonitor.Notifications send core, so the filter is one +/// fix for both SKUs; Lite.Tests.IncidentCooldownTests pins the decision side of the seam. +/// +public sealed class IncidentDeliveryFilterTests +{ + /* Fingerprint shapes from the measured case, truncated the way the report quotes them. */ + private const string Stale = "bf7e29b4cooldowninwindow"; + private const string Fresh = "9d5ead0aoutsideitswindow"; + + private const string Footer = "1 other incident(s) still open (already reported)"; + + private static readonly AlertBranding Branding = new("Test Edition", null); + + /* ─────────────── the filter itself ─────────────── */ + + /// + /// The defect, at the seam. Two fingerprints, one of them already delivered and still inside its own + /// window: the render carries the fresh one, no item names the stale one, and the count of what was held + /// back is stated rather than silently dropped. + /// + [Fact] + public void AMixedBatch_RendersOnlyTheDeliverableIncident_AndFootnotesTheRest() + { + var context = TwoIncidents(); + + var render = IncidentDeliveryFilter.ForDelivery(context, null, new[] { Fresh }); + + Assert.Equal(1, render.SuppressedIncidentCount); + Assert.NotSame(context, render.Context); + + var incident = Assert.Single(render.Context!.Incidents!); + Assert.Equal(Fresh, incident.DedupKey); + + Assert.DoesNotContain(render.Context.Details, DedupKeyIs(Stale)); + Assert.Contains(render.Context.Details, DedupKeyIs(Fresh)); + + var footer = Assert.Single( + render.Context.Details, d => d.Heading == IncidentDeliveryFilter.OtherIncidentsHeading); + Assert.Contains(footer.Fields, f => f.Value == Footer); + } + + /// + /// The control, and #1154's whole point: a batch whose every fingerprint is outside its window delivers + /// all of them. Asserted by REFERENCE — an unfiltered alert renders the very instance the caller built, + /// so the four channels and the email template see byte-for-byte what they saw before #3313, and a + /// filter that rebuilt the context unconditionally (and could therefore drop something on the common + /// path) fails here rather than in whichever channel assertion happened to notice. + /// + [Fact] + public void ABatchWhoseEveryFingerprintIsFresh_IsReturnedUnfiltered() + { + var context = TwoIncidents(); + + var render = IncidentDeliveryFilter.ForDelivery(context, null, new[] { Stale, Fresh }); + + Assert.Same(context, render.Context); + Assert.Equal(0, render.SuppressedIncidentCount); + Assert.DoesNotContain( + render.Context!.Details, d => d.Heading == IncidentDeliveryFilter.OtherIncidentsHeading); + } + + /// + /// Delivery filters; persistence does not. The caller writes its config_alert_log row from the + /// context it still holds AFTER the send, so a filter that mutated in place would silently strip the + /// history row, the context JSON, the triage page, the Viewer's detail pane and + /// AlertMuteContext.PopulateFromDetailText of everything it held back. + /// + [Fact] + public void TheSourceContext_IsNeverMutated() + { + var context = TwoIncidents(); + var itemCount = context.Details.Count; + + IncidentDeliveryFilter.ForDelivery(context, null, new[] { Fresh }); + + Assert.Equal(2, context.Incidents!.Count); + Assert.Equal(itemCount, context.Details.Count); + Assert.Contains(context.Details, DedupKeyIs(Stale)); + Assert.DoesNotContain(context.Details, d => d.Heading == IncidentDeliveryFilter.OtherIncidentsHeading); + } + + /// + /// An alert with no fingerprintable incident is evaluated on the metric-level fallback key, so its + /// deliverable set is null — "nothing to filter", not "nothing to show". That is the shape of + /// every CPU / memory / poison-wait / tempdb / failed-job alert AND of the #2109 AG database alerts, + /// whose SET HADR RESUME prose is the one thing the delivery gate was deliberately built not to + /// drop: AgAlertContexts.ForDatabase builds one plain detail item and never touches + /// , so the filter cannot reach it even in principle. Pinned + /// anyway — that is a property of a file this change does not touch, and nothing else would notice if + /// it changed. + /// + [Fact] + public void AnAlertWithNoFingerprints_IsUnfiltered_AndKeepsItsRemediationProse() + { + const string Remedy = "Fix the underlying cause, then resume it with ALTER DATABASE [Sales] SET HADR RESUME."; + var context = AgAlertContexts.ForDatabase("Sales", "ag-primary", "replica-a", ("Suspend Reason", "SUSPEND_FROM_USER")); + + var render = IncidentDeliveryFilter.ForDelivery(context, Remedy, deliverableDedupKeys: null); + + Assert.Same(context, render.Context); + Assert.Equal(Remedy, render.Prose); + Assert.Equal(0, render.SuppressedIncidentCount); + } + + /// + /// Only an incident's OWN item is dropped. Every other item describes the whole alert — the builder's + /// data section, advice prose, the remediation T-SQL block, and the standalone "Deadlock Victim" item + /// BuildDeadlockContext keeps for a deadlock whose objects would not parse — and none of them + /// belongs to a fingerprint, so none of them is a fingerprint's to take away. + /// + [Fact] + public void ItemsThatBelongToNoIncident_AreAlwaysKept() + { + var context = new AlertContext(); + context.Details.Add(new AlertDetailItem { Heading = "Deadlock Victim", Fields = new() { ("Victim SQL", "SELECT 1") } }); + context.Details.Add(new AlertDetailItem { Heading = "What to check", Body = "Investigation: look at the graph." }); + context.Details.Add(new AlertDetailItem { Heading = "Remediation", IsCodeBlock = true, Body = "-- script" }); + AlertIncidentRenderer.Apply(context, Incidents()); + + var render = IncidentDeliveryFilter.ForDelivery(context, null, new[] { Fresh }); + + Assert.Contains(render.Context!.Details, d => d.Heading == "Deadlock Victim"); + Assert.Contains(render.Context.Details, d => d.Heading == "What to check"); + Assert.Contains(render.Context.Details, d => d.Heading == "Remediation" && d.IsCodeBlock); + } + + /// + /// An incident with a blank fingerprint produces no cooldown key at all (BuildKeys excludes it), + /// so it can never have been throttled — and must never be filtered out on a key it does not have. It + /// would otherwise vanish from every card the moment a fingerprinted sibling went into cooldown, which + /// is the same class of silent loss #3313 is about, pointed the other way. + /// + [Fact] + public void AnIncidentWithNoFingerprint_IsNeverFilteredOut() + { + var context = new AlertContext(); + AlertIncidentRenderer.Apply(context, new[] + { + new AlertIncident(Stale, new[] { "SalesDb.dbo.Orders" }), + new AlertIncident("", new[] { "SalesDb.dbo.Unparsed" }), + new AlertIncident(Fresh, new[] { "SalesDb.dbo.Shipments" }) + }); + + var render = IncidentDeliveryFilter.ForDelivery(context, null, new[] { Fresh }); + + Assert.Equal(2, render.Context!.Incidents!.Count); + Assert.Contains(render.Context.Incidents!, i => i.DedupKey == ""); + Assert.Contains(render.Context.Incidents!, i => i.DedupKey == Fresh); + } + + /// + /// A deliverable set naming none of the alert's fingerprints renders the WHOLE alert rather than an + /// empty card. It cannot arise from a ShouldSend decision — that says at least one key was fresh + /// — so it means a caller paired a context with another evaluation's key set, and the two ways to be + /// wrong there are not symmetric: repeating an incident is noise, while posting a card with no incident + /// on it destroys the alert. + /// + [Fact] + public void ADeliverableSetMatchingNothing_RendersTheWholeAlert() + { + var context = TwoIncidents(); + + var render = IncidentDeliveryFilter.ForDelivery(context, null, Array.Empty()); + + Assert.Same(context, render.Context); + Assert.Equal(0, render.SuppressedIncidentCount); + } + + /// + /// The filter finds an incident's item by the fact name + /// emits, so the two must be reading and writing the same string. Read off the SHIPPED item rather than + /// compared to a literal: a renamed fact with both sides renamed together is fine, while a literal in + /// the filter that drifted from the renderer would leave every item unmatched and the filter silently + /// dropping nothing at all. + /// + [Fact] + public void TheFilterMatchesOnTheFactNameTheRendererEmits() + { + var item = AlertIncidentRenderer.BuildItem( + new AlertIncident(Stale, new[] { "SalesDb.dbo.Orders" }), "Deadlock", includeDetailFields: false); + + Assert.Contains(item.Fields, f => f.Label == AlertIncidentRenderer.DedupKeyFactName && f.Value == Stale); + } + + /* ─────────────── the wire ─────────────── */ + + /// + /// The pin that matters. A Summary-mode batch carrying one fingerprint inside its window and one + /// outside delivers only the fresh one on every channel, while the history row carries both. + /// + /// Teams, Slack and the generic channel all point at one loopback endpoint; PagerDuty's endpoint + /// is the hardcoded Events v2 URL and cannot be redirected, and it takes the same filtered context from + /// the same single call in the fan-out as the three checked here. Email is asserted too, because it + /// resolves its own decision against its own key space and its own filter call — a fix applied to the + /// webhook fan-out alone would pass every webhook assertion here. + /// + [Fact] + public async Task ASummaryBatch_DeliversOnlyTheFreshFingerprint_WhileTheHistoryRowCarriesBoth() + { + using var endpoint = new CapturingWebhookEndpoint(); + using var smtp = new CapturingSmtpEndpoint(); + + var history = new SeedingHistoryStore(Stale, DateTime.UtcNow - TimeSpan.FromMinutes(1)); + var (deliverer, _) = BuildDeliverer(endpoint, smtp, history); + + var context = TwoIncidents(); + await deliverer.DeliverAsync(Outcome(context), TestContext.Current.CancellationToken); + + var bodies = endpoint.Bodies; + Assert.Equal(3, bodies.Count); + Assert.All(bodies, body => Assert.Contains(Fresh, body, StringComparison.Ordinal)); + Assert.All(bodies, body => Assert.DoesNotContain(Stale, body, StringComparison.Ordinal)); + Assert.All(bodies, body => Assert.Contains(Footer, body, StringComparison.Ordinal)); + + var message = Assert.Single(smtp.Messages); + Assert.Contains(Fresh, message, StringComparison.Ordinal); + Assert.DoesNotContain(Stale, message, StringComparison.Ordinal); + Assert.Contains(Footer, message, StringComparison.Ordinal); + + /* And the row is whole: both fingerprints in the flat detail AND in the context JSON, which are two + separately-written columns read by different consumers. */ + var record = Assert.Single(history.Records); + Assert.Contains(Stale, record.DetailText!, StringComparison.Ordinal); + Assert.Contains(Fresh, record.DetailText!, StringComparison.Ordinal); + Assert.Contains(Stale, record.ContextJson!, StringComparison.Ordinal); + Assert.Contains(Fresh, record.ContextJson!, StringComparison.Ordinal); + Assert.DoesNotContain(Footer, record.DetailText!, StringComparison.Ordinal); + } + + /// + /// The control on the wire: a batch whose every fingerprint is fresh still delivers all of them, with no + /// footer. Without it, "deliver only the fresh one" is satisfied by a filter that keeps exactly one + /// incident per card forever. + /// + [Fact] + public async Task ABatchWhoseEveryFingerprintIsFresh_DeliversAllOfThem() + { + using var endpoint = new CapturingWebhookEndpoint(); + using var smtp = new CapturingSmtpEndpoint(); + + var history = new SeedingHistoryStore(seededDedupKey: null, DateTime.UtcNow); + var (deliverer, _) = BuildDeliverer(endpoint, smtp, history); + + await deliverer.DeliverAsync(Outcome(TwoIncidents()), TestContext.Current.CancellationToken); + + var bodies = endpoint.Bodies; + Assert.Equal(3, bodies.Count); + Assert.All(bodies, body => Assert.Contains(Stale, body, StringComparison.Ordinal)); + Assert.All(bodies, body => Assert.Contains(Fresh, body, StringComparison.Ordinal)); + Assert.All(bodies, body => Assert.DoesNotContain( + IncidentDeliveryFilter.OtherIncidentsHeading, body, StringComparison.Ordinal)); + + var message = Assert.Single(smtp.Messages); + Assert.Contains(Stale, message, StringComparison.Ordinal); + Assert.Contains(Fresh, message, StringComparison.Ordinal); + } + + /// + /// A prose detail that is NOT a flattening of the alert's own context still reaches the wire on a + /// FILTERED card — the #2109 SET HADR RESUME case, put on the one shape no existing pin covers + /// (prose and fingerprinted incidents on the same alert). + /// + /// This is the interaction worth pinning rather than arguing. Five channel builders re-run + /// AlertDetailText.ProseForDelivery against whatever context they are handed; the prose decision + /// is therefore made once, against the UNFILTERED context, since a comparison against the filtered copy + /// would find them unequal and print the whole flattened alert — resurrecting in prose exactly the + /// incidents just removed from the structure. + /// + [Fact] + public async Task AnIndependentProse_StillReachesTheWire_OnAFilteredCard() + { + const string Remedy = "Fix the underlying cause, then resume it with ALTER DATABASE [Sales] SET HADR RESUME."; + + using var endpoint = new CapturingWebhookEndpoint(); + using var smtp = new CapturingSmtpEndpoint(); + + var history = new SeedingHistoryStore(Stale, DateTime.UtcNow - TimeSpan.FromMinutes(1)); + var (deliverer, _) = BuildDeliverer(endpoint, smtp, history); + + var context = TwoIncidents(); + await deliverer.DeliverAsync( + Outcome(context) with { DetailText = Remedy }, TestContext.Current.CancellationToken); + + Assert.Equal(3, endpoint.Bodies.Count); + Assert.All(endpoint.Bodies, body => Assert.Contains("SET HADR RESUME", body, StringComparison.Ordinal)); + Assert.All(endpoint.Bodies, body => Assert.DoesNotContain(Stale, body, StringComparison.Ordinal)); + Assert.Contains("SET HADR RESUME", Assert.Single(smtp.Messages), StringComparison.Ordinal); + } + + /// + /// Per-event mode (#1141) does not have this defect and is not changed, established here rather than + /// asserted from reading: PerEventNotification.Split gives each message a single-incident + /// context, so an in-window fingerprint fails its OWN ShouldSend and never posts at all. One card + /// set goes out for the fresh incident and none for the stale one — while both still get a history row, + /// because per-event records once per split message whether or not a channel accepted it. + /// + [Fact] + public async Task PerEventMode_PostsTheFreshFingerprintOnly_AndRecordsBoth() + { + using var endpoint = new CapturingWebhookEndpoint(); + using var smtp = new CapturingSmtpEndpoint(); + + var history = new SeedingHistoryStore(Stale, DateTime.UtcNow - TimeSpan.FromMinutes(1)); + var (deliverer, _) = BuildDeliverer( + endpoint, smtp, history, config => config.Alerts.DeliveryMode = AlertNotificationMode.PerEvent); + + await deliverer.DeliverAsync(Outcome(TwoIncidents()), TestContext.Current.CancellationToken); + + Assert.Equal(3, endpoint.Bodies.Count); + Assert.All(endpoint.Bodies, body => Assert.Contains(Fresh, body, StringComparison.Ordinal)); + Assert.All(endpoint.Bodies, body => Assert.DoesNotContain(Stale, body, StringComparison.Ordinal)); + + /* No footer: a single-incident card held nothing back — the cooldown refused the other message + outright, which is a different and already-correct mechanism. */ + Assert.All(endpoint.Bodies, body => Assert.DoesNotContain( + IncidentDeliveryFilter.OtherIncidentsHeading, body, StringComparison.Ordinal)); + + Assert.Equal(2, history.Records.Count); + Assert.Contains(history.Records, r => r.DetailText!.Contains(Stale, StringComparison.Ordinal)); + Assert.Contains(history.Records, r => r.DetailText!.Contains(Fresh, StringComparison.Ordinal)); + } + + /* ─────────────── helpers ─────────────── */ + + private static IReadOnlyList Incidents() => new[] + { + /* TotalOccurrences: 1 on both — the measured card's proof that the repeat was a re-render and not a + recurrence, carried here so the fixture is the reported shape rather than a convenient one. */ + new AlertIncident(Stale, new[] { "SalesDb.dbo.Orders" }, TotalOccurrences: 1), + new AlertIncident(Fresh, new[] { "SalesDb.dbo.Shipments" }, TotalOccurrences: 1) + }; + + private static AlertContext TwoIncidents() + { + var context = new AlertContext(); + AlertIncidentRenderer.Apply(context, Incidents()); + return context; + } + + /* The engine's own shape: DetailText is ContextToDetailText of the context, which is what makes + ProseForDelivery recognise it as a restatement and suppress it. */ + private static AlertOutcome Outcome(AlertContext context) => new( + "7", "PROD01", "Deadlocks Detected", "2", "1", + context, AlertContextBuilders.ContextToDetailText(context), + NumericCurrentValue: 2, NumericThresholdValue: 1, Muted: false, Severity: null); + + private static (DarlingAlertDeliverer Deliverer, DarlingAlertSettings Settings) BuildDeliverer( + CapturingWebhookEndpoint endpoint, + CapturingSmtpEndpoint smtp, + IAlertHistoryStore history, + Action? configure = null) + { + var config = new DarlingConfig(); + config.Webhooks.TeamsUrl = endpoint.Url; + config.Webhooks.SlackUrl = endpoint.Url; + config.Webhooks.GenericUrl = endpoint.Url; + config.Smtp.Host = "127.0.0.1"; + config.Smtp.Port = smtp.Port; + config.Smtp.UseSsl = false; + config.Smtp.From = "monitor@example.invalid"; + config.Smtp.To = "operator@example.invalid"; + configure?.Invoke(config); + + var settings = new DarlingAlertSettings(config); + var webhooks = new WebhookAlertService( + settings, DarlingAlertDeliverer.Branding, NullLogger.Instance, history); + return (new DarlingAlertDeliverer(settings, history, webhooks, NullLogger.Instance), settings); + } + + private static Predicate DedupKeyIs(string dedupKey) => + item => item.Fields.Any( + f => f.Label == AlertIncidentRenderer.DedupKeyFactName && f.Value == dedupKey); + + /// + /// Reports one named fingerprint as sent at on BOTH channels, and null for + /// everything else — the restart-seed path (#981/#1145) is how a cooldown learns an incident was already + /// delivered, and it is also the only way to put a key in the window without sleeping through one. + /// Both channels, because email and webhook hold separate key spaces and seeding one would leave the + /// other's filter with nothing to do. + /// + private sealed class SeedingHistoryStore : IAlertHistoryStore + { + private readonly string? _seededDedupKey; + private readonly DateTime _sentUtc; + + public SeedingHistoryStore(string? seededDedupKey, DateTime sentUtc) + { + _seededDedupKey = seededDedupKey; + _sentUtc = sentUtc; + } + + public List Records { get; } = new(); + + public Task RecordAlertAsync(AlertHistoryRecord record) + { + Records.Add(record); + return Task.CompletedTask; + } + + public Task GetLastEmailSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => + Seed(dedupKey); + + public Task GetLastWebhookSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => + Seed(dedupKey); + + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => + Seed(dedupKey); + + private Task Seed(string? dedupKey) => + Task.FromResult( + _seededDedupKey is not null && string.Equals(dedupKey, _seededDedupKey, StringComparison.Ordinal) + ? _sentUtc + : null); + } +} diff --git a/Lite.Tests/AlertIncidentRenderTests.cs b/Lite.Tests/AlertIncidentRenderTests.cs index ac15b9650c..ad2024beea 100644 --- a/Lite.Tests/AlertIncidentRenderTests.cs +++ b/Lite.Tests/AlertIncidentRenderTests.cs @@ -111,6 +111,45 @@ public void TeamsPayload_EachFieldsItem_GetsItsOwnLabeledSection() f => f.GetProperty("value").GetString() == "bbb"); } + /// + /// #3313: the delivery filter's output renders correctly on every shared surface. The filter removes a + /// stale incident's item and appends a one-line footer; this pins that all four renderers — Teams facts, + /// Slack fields, and the two independently-built email bodies — show the surviving fingerprint and the + /// footer, and none of them shows the stale one. + /// + /// Lite and Darling share these builders, so a delivery pin on one SKU's service says nothing + /// about whether the other's renderers handle the shape. Read from the filter's own constants rather + /// than a copy of its text: a footer whose wording changed while this pin kept passing against the old + /// string would be asserting nothing. + /// + [Fact] + public void AFilteredContext_RendersItsSurvivorAndFooter_OnEveryVehicle() + { + var ctx = new AlertContext(); + AlertIncidentRenderer.Apply(ctx, new[] + { + new AlertIncident("stale-fingerprint", new[] { "SalesDb.dbo.Orders" }), + new AlertIncident("fresh-fingerprint", new[] { "SalesDb.dbo.Shipments" }) + }); + + var render = IncidentDeliveryFilter.ForDelivery(ctx, null, new[] { "fresh-fingerprint" }); + var filtered = render.Context; + Assert.Equal(1, render.SuppressedIncidentCount); + + var teams = WebhookAlertService.BuildTeamsPayload("Deadlocks Detected", "S1", "2", "n/a", Branding, context: filtered); + var slack = WebhookAlertService.BuildSlackPayload("Deadlocks Detected", "S1", "2", "n/a", Branding, context: filtered); + var (html, plain) = EmailTemplateBuilder.BuildAlertEmail( + "Deadlocks Detected", "S1", "2", "n/a", 15, Branding, filtered); + + foreach (var vehicle in new[] { teams, slack, html, plain }) + { + Assert.Contains("fresh-fingerprint", vehicle); + Assert.DoesNotContain("stale-fingerprint", vehicle); + Assert.Contains(IncidentDeliveryFilter.OtherIncidentsHeading, vehicle); + Assert.Contains(IncidentDeliveryFilter.OtherIncidentsLabel, vehicle); + } + } + [Fact] public void DedupKey_RendersOnTeamsSlackAndBothEmailBodies() { diff --git a/Lite.Tests/IncidentCooldownTests.cs b/Lite.Tests/IncidentCooldownTests.cs index df95a1b60e..001954f86e 100644 --- a/Lite.Tests/IncidentCooldownTests.cs +++ b/Lite.Tests/IncidentCooldownTests.cs @@ -151,6 +151,70 @@ public async Task FailedSend_NotStamped_DoesNotStartCooldown() Assert.True((await cd.EvaluateAsync("1", "Deadlocks Detected", Incidents("A"), Window)).ShouldSend); } + /* ─────────────── #3313: the per-fingerprint verdicts, not only their reduction ─────────────── */ + + /// + /// #3313: the decision reports WHICH fingerprints were outside their own window, not only that one of + /// them was. The "send if ANY is fresh" reduction above is the right answer to "does this post" and was + /// the only answer available, so the channel builders rendered the whole incident set — including the + /// ones still inside their own window, already delivered minutes earlier. + /// + /// Reported as a member of the same decision rather than left to a second call: the cooldown + /// evicts, seeds from history and stamps, so a render evaluating freshness for itself would not be + /// asking the same question the send decision answered. Consumed by + /// IncidentDeliveryFilter.ForDelivery; the render side is pinned in + /// Darling.Tests.IncidentDeliveryFilterTests. + /// + [Fact] + public async Task Decision_NamesOnlyTheFingerprintsOutsideTheirOwnWindow() + { + var cd = NoSeed(); + + var a = await cd.EvaluateAsync("1", "Deadlocks Detected", Incidents("A"), Window); + cd.Stamp(a); + Assert.Equal(new[] { "A" }, a.DeliverableDedupKeys); + + // Summary {A (in cooldown), B (fresh)}: posts because B is fresh, but only B is deliverable. + var summary = await cd.EvaluateAsync("1", "Deadlocks Detected", Incidents("A", "B"), Window); + Assert.True(summary.ShouldSend); + Assert.Equal(2, summary.Keys.Count); // both keys are still stamped on success (#1154) + Assert.Equal(new[] { "B" }, summary.DeliverableDedupKeys); + } + + /// + /// The metric-level fallback reports null, not an empty list. Empty would read as "no incident is + /// deliverable" and blank every CPU / memory / poison-wait / tempdb / failed-job card, plus the #2109 AG + /// database alerts — none of which carries a fingerprint at all. + /// + [Fact] + public async Task Decision_ReportsNoDeliverableSet_WhenThereAreNoFingerprints() + { + var cd = NoSeed(); + + Assert.Null((await cd.EvaluateAsync("1", "High CPU", null, Window)).DeliverableDedupKeys); + Assert.Null((await cd.EvaluateAsync("1", "High CPU", new List(), Window)).DeliverableDedupKeys); + } + + /// + /// A stamped-and-suppressed alert reports an EMPTY deliverable set, which is the state the null above + /// has to stay distinguishable from. Unreachable through a send (ShouldSend is false, so nothing + /// renders), and pinned for exactly that reason: it is the only pairing of the two members a renderer + /// must never see, so nothing else would notice the two collapsing into one value. + /// + [Fact] + public async Task Decision_ReportsAnEmptyDeliverableSet_WhenEveryFingerprintIsSuppressed() + { + var cd = NoSeed(); + + var first = await cd.EvaluateAsync("1", "Deadlocks Detected", Incidents("A", "B"), Window); + cd.Stamp(first); + + var again = await cd.EvaluateAsync("1", "Deadlocks Detected", Incidents("A", "B"), Window); + Assert.False(again.ShouldSend); + Assert.NotNull(again.DeliverableDedupKeys); + Assert.Empty(again.DeliverableDedupKeys!); + } + [Fact] public async Task Eviction_DropsKeysPastTwiceWindow_KeepsDictBounded() { diff --git a/PerformanceMonitor.Notifications/AlertIncidentRenderer.cs b/PerformanceMonitor.Notifications/AlertIncidentRenderer.cs index 105aba9258..883f9a2ce5 100644 --- a/PerformanceMonitor.Notifications/AlertIncidentRenderer.cs +++ b/PerformanceMonitor.Notifications/AlertIncidentRenderer.cs @@ -26,6 +26,14 @@ namespace PerformanceMonitor.Notifications; /// public static class AlertIncidentRenderer { + /// + /// The fact name carrying an incident's fingerprint. Named here because it is the only thing tying a + /// rendered back to the incident it came from: #3313's delivery filter + /// drops a stale incident's item by matching on it, and a second copy of the string would be free to + /// drift from the one actually emits. + /// + public const string DedupKeyFactName = "Dedup Key"; + /// /// Sets .Incidents and appends one detail item per incident. No-op when /// there are no incidents. Call once from each alert builder after computing the incidents. @@ -60,7 +68,7 @@ public static AlertDetailItem BuildItem(AlertIncident incident, string heading, foreach (var f in incident.DetailFields) item.Fields.Add((f.Label, f.Value)); } - item.Fields.Add(("Dedup Key", incident.DedupKey)); + item.Fields.Add((DedupKeyFactName, incident.DedupKey)); item.Fields.Add(("Involved Objects", incident.InvolvedObjects.Count > 0 ? string.Join(", ", incident.InvolvedObjects) : "(unresolved)")); if (incident.OccurrenceCount > 1) diff --git a/PerformanceMonitor.Notifications/EmailSendCore.cs b/PerformanceMonitor.Notifications/EmailSendCore.cs index 76a078d2c7..745b7a2684 100644 --- a/PerformanceMonitor.Notifications/EmailSendCore.cs +++ b/PerformanceMonitor.Notifications/EmailSendCore.cs @@ -115,7 +115,8 @@ public async Task TrySendAsync( /* #1154: per-fingerprint cooldown. Send if any incident in this alert is outside its window (a distinct fingerprint is not throttled by an unrelated prior incident); stamp every candidate key only after a successful send. Seeds from the alert log on - first touch per key (#981). No incidents -> the metric-level fallback key (today's behavior). */ + first touch per key (#981). No incidents -> the metric-level fallback key (today's behavior). + WHETHER to send only; #3313's filter below decides WHICH incidents the email contains. */ var decision = await _cooldown.EvaluateAsync( serverId, metricName, context?.Incidents, TimeSpan.FromMinutes(_settings.EmailCooldownMinutes)); @@ -124,15 +125,21 @@ first touch per key (#981). No incidents -> the metric-level fallback key (today { emailAttempted = true; + /* #3313: render only the incidents outside their own window. Email keys off THIS path's own + cooldown, not the webhook's: the two channels hold separate key spaces (see the keyPrefix + on each IncidentCooldown), so an email that failed to send last cycle left its key + unstamped and its incident is still owed a delivery even where the webhook's is not. */ + var render = IncidentDeliveryFilter.ForDelivery(context, detailText, decision.DeliverableDedupKeys); + var titleName = string.IsNullOrEmpty(displayName) ? metricName : displayName; var subject = $"[SQL Monitor Alert] {titleName} on {serverName}"; var (htmlBody, plainTextBody) = EmailTemplateBuilder.BuildAlertEmail( - metricName, serverName, currentValue, thresholdValue, _settings.EmailCooldownMinutes, _branding, context, - detailText, displayName); + metricName, serverName, currentValue, thresholdValue, _settings.EmailCooldownMinutes, _branding, + render.Context, render.Prose, displayName); try { - await SendEmailAsync(_settings, subject, htmlBody, plainTextBody, context); + await SendEmailAsync(_settings, subject, htmlBody, plainTextBody, render.Context); emailSent = true; _cooldown.Stamp(decision); @@ -163,7 +170,9 @@ first touch per key (#981). No incidents -> the metric-level fallback key (today } } - /* Webhook notifications (Teams / Slack) — independent of email */ + /* Webhook notifications (Teams / Slack) — independent of email, and handed the UNFILTERED context: + it owns its own cooldown key space and applies its own #3313 filter from its own decision. Passing + email's filtered copy would make one channel's send history govern the other's card. */ bool webhookSent = false; if (attemptChannels) { diff --git a/PerformanceMonitor.Notifications/IncidentCooldown.cs b/PerformanceMonitor.Notifications/IncidentCooldown.cs index 096788ce8f..e349b8363a 100644 --- a/PerformanceMonitor.Notifications/IncidentCooldown.cs +++ b/PerformanceMonitor.Notifications/IncidentCooldown.cs @@ -51,6 +51,11 @@ public IncidentCooldown(string keyPrefix, Func#3313: the per-fingerprint verdicts the "at least one" reduction is computed FROM are reported + /// as , because "rode along on a fresh sibling" is the + /// behaviour that made an already-delivered incident reappear on the next card. The reduction stays — + /// posting is still all-or-nothing per alert — but the render no longer has to treat it as the only + /// answer available. /// public async Task EvaluateAsync( string serverId, string metricName, IReadOnlyList? incidents, TimeSpan window) @@ -61,6 +66,9 @@ public async Task EvaluateAsync( var keys = BuildKeys(serverId, metricName, incidents); bool anyFresh = false; + bool fingerprinted = false; + var deliverable = new List(keys.Count); + foreach (var (key, dedupKey) in keys) { if (_seedLastSentUtc is not null && !_cooldowns.ContainsKey(key)) @@ -70,11 +78,23 @@ public async Task EvaluateAsync( _cooldowns.TryAdd(key, lastSent.Value); } - if (!_cooldowns.TryGetValue(key, out var last) || now - last >= window) + var fresh = !_cooldowns.TryGetValue(key, out var last) || now - last >= window; + if (fresh) anyFresh = true; + + if (dedupKey is not null) + { + fingerprinted = true; + if (fresh) + deliverable.Add(dedupKey); + } } - return new Decision(anyFresh, keys.Select(k => k.Key).ToList(), now); + /* null, not empty, on the metric-level fallback: "this alert has no fingerprints to filter" and + "every one of its fingerprints is in cooldown" are different answers, and the render has to be + able to tell them apart (an empty list would read as "show nothing"). */ + return new Decision( + anyFresh, keys.Select(k => k.Key).ToList(), now, fingerprinted ? deliverable : null); } /// @@ -126,5 +146,18 @@ private void Evict(DateTime now, TimeSpan window) internal int TrackedKeyCount => _cooldowns.Count; /// The send decision plus the candidate keys to stamp on a successful send. - public sealed record Decision(bool ShouldSend, IReadOnlyList Keys, DateTime EvaluatedAtUtc); + /// + /// #3313: the #1140 dedup fingerprints that were OUTSIDE their own window — the incidents a channel + /// should render, as opposed to the whole set the alert happens to carry. null when the alert + /// carried no fingerprint and was evaluated on the metric-level fallback key, which is "nothing to + /// filter" rather than "nothing to show". Fed to . + /// A member of the same value as rather than a second call the caller + /// has to remember to make: the two answers come from one pass over one key set, and a render deciding + /// freshness for itself could disagree with the decision that let it post. + /// + public sealed record Decision( + bool ShouldSend, + IReadOnlyList Keys, + DateTime EvaluatedAtUtc, + IReadOnlyList? DeliverableDedupKeys); } diff --git a/PerformanceMonitor.Notifications/IncidentDeliveryFilter.cs b/PerformanceMonitor.Notifications/IncidentDeliveryFilter.cs new file mode 100644 index 0000000000..2edcb9c874 --- /dev/null +++ b/PerformanceMonitor.Notifications/IncidentDeliveryFilter.cs @@ -0,0 +1,201 @@ +/* + * 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; + +namespace PerformanceMonitor.Notifications; + +/// +/// #3313: narrows an alert to the incidents a channel should actually RENDER, given the per-fingerprint +/// decision that let the alert post at all. +/// +/// answers "does this alert post", and #1154 makes that answer "yes if ANY +/// fingerprint is outside its own window" so a genuinely distinct incident is never throttled by an +/// unrelated prior one. Nothing answered the second question — WHICH incidents the card then contains — so +/// the channel builders rendered every entry in , including ones still +/// inside their own window that had been delivered minutes earlier. One new fingerprint dragged every +/// co-resident stale fingerprint back into the channel with it. Measured on a 42-server store: a card +/// delivered fingerprint bf7e29b4 alone, then 41 minutes later re-delivered it as "Deadlock 2 of 2" +/// riding on a new fingerprint, with Total Occurrences: 1 proving it had not recurred — it was simply +/// still inside the rolling read window. Raising the cooldown cannot fix that: the repeat is a passenger on +/// another fingerprint's card, not a second firing of its own. +/// +/// +/// Delivery filters; persistence does not. This returns a COPY and never mutates its input, so the +/// config_alert_log row its caller writes afterwards — detail_text and the context JSON the MCP +/// reader, the triage page, the Viewer's detail pane and AlertMuteContext.PopulateFromDetailText all +/// read — still carries every incident. The same split #3317 shipped for analysis prose +/// (EmailAlertService's deliverProse), for the same reason. +/// +/// +/// Per-event mode (#1141) does not have the defect and is not changed. +/// PerEventNotification.Split already emits one message per incident, each carrying a single-incident +/// context, and each goes through its own send — so an in-window fingerprint fails its own +/// and never posts. Its trailing "+N more" overflow +/// message is the one per-event payload carrying several fingerprints, and it is filtered here like any +/// other batch because this sits at the delivery boundary every message crosses. +/// +/// +public static class IncidentDeliveryFilter +{ + /// + /// Heading of the one-line footer standing in for the omitted incidents. A footer rather than silence: + /// an operator reading a one-incident card needs to know the server still has others open, and the + /// alternative — restating each of them — is the defect. + /// + public const string OtherIncidentsHeading = "Other Incidents"; + + /// Label of the footer's single fact. A fact name is a consumer API (see + /// ), so it is declared here rather than spelled inline. + public const string OtherIncidentsLabel = "Still Open"; + + /// + /// The delivery-scoped view of an alert. + /// + /// + /// What the channel builders should render: the input instance itself when nothing is filtered (so an + /// unfiltered alert is byte-identical to pre-#3313), otherwise a copy carrying the deliverable incidents, + /// their detail items, every non-incident item, and the footer. + /// + /// + /// The alert's flat prose detail to render, resolved through + /// against the UNFILTERED context — see the remarks on for why that basis is + /// the only correct one. + /// + /// + /// How many incidents were held back, so a caller can log or count them. Zero whenever + /// is the input instance. + /// + public readonly record struct Render(AlertContext? Context, string? Prose, int SuppressedIncidentCount); + + /// + /// Filters down to the incidents named by + /// and resolves the prose to deliver alongside them. + /// + /// is : + /// the fingerprints that were outside their own window at evaluation time. null means the alert + /// carried no fingerprint at all and was evaluated on the metric-level fallback key (CPU, memory, + /// poison wait, tempdb, failed job, and the #2109 AG database alerts, whose context is one plain detail + /// item and no incidents) — there is nothing to filter, so the input is returned unchanged. + /// + /// + /// The prose is resolved against the unfiltered context, and that is load-bearing. Every engine + /// alert's detailText IS of its own context, which is how + /// recognises it as a restatement and suppresses it. + /// Comparing it against the FILTERED copy instead would find them unequal and print the whole flattened + /// alert as prose — resurrecting, in prose, exactly the stale incidents just removed from the structure. + /// + /// + /// Five channel builders re-run on whatever prose they are + /// handed, against the context they are handed. That is a no-op on a null prose and on any prose that + /// differs from that context's own flatten — but rather than leave it argued, the one shape that would + /// break it (a prose equal to the FILTERED copy's flatten) takes the unfiltered path below. Failing that + /// way costs a repeated incident; failing the other way would discard an operator's only copy of a + /// remedy, which is #3296. + /// + /// + public static Render ForDelivery( + AlertContext? context, string? detailText, IReadOnlyList? deliverableDedupKeys) + { + var prose = AlertDetailText.ProseForDelivery(detailText, context); + + if (context is null || deliverableDedupKeys is null || context.Incidents is not { Count: > 0 } incidents) + { + return new Render(context, prose, 0); + } + + var deliverable = new HashSet(deliverableDedupKeys, StringComparer.Ordinal); + + var kept = new List(incidents.Count); + var suppressedKeys = new HashSet(StringComparer.Ordinal); + foreach (var incident in incidents) + { + /* A blank DedupKey produces no cooldown key at all (IncidentCooldown.BuildKeys excludes it), so + it can never have been throttled and must never be filtered out on a key it does not have. */ + if (string.IsNullOrEmpty(incident.DedupKey) || deliverable.Contains(incident.DedupKey)) + { + kept.Add(incident); + continue; + } + + suppressedKeys.Add(incident.DedupKey); + } + + /* Nothing to hold back, or nothing left to show. The second case cannot arise from a ShouldSend + decision (it says at least one key was fresh), so it means the caller paired a context with + another evaluation's key set; rendering the whole alert is the direction that over-reports rather + than delivering an empty card. */ + if (suppressedKeys.Count == 0 || kept.Count == 0) + { + return new Render(context, prose, 0); + } + + var suppressedCount = incidents.Count - kept.Count; + + var rendered = new AlertContext + { + /* Carried over for the same reasons PerEventNotification.NewContext carries them: severity + drives every channel's accent, and the attachment is the forensic deadlock/blocked-process XML + the email keeps. */ + SeverityOverride = context.SeverityOverride, + AttachmentXml = context.AttachmentXml, + AttachmentFileName = context.AttachmentFileName, + Incidents = kept + }; + + /* An incident's detail item is identified by the Dedup Key fact AlertIncidentRenderer.BuildItem + always emits — the sole producer of that fact name in the tree, and the name itself comes from + there rather than a copy of the string. Every other item (advice prose, remediation T-SQL, a + deadlock victim the fingerprint could not parse, a builder's own data section) carries no such + fact and is kept: those describe the whole alert, not one fingerprint. */ + foreach (var item in context.Details) + { + if (!NamesAnyOf(item, suppressedKeys)) + { + rendered.Details.Add(item); + } + } + + rendered.Details.Add(new AlertDetailItem + { + Heading = OtherIncidentsHeading, + Fields = new List<(string Label, string Value)> + { + (OtherIncidentsLabel, + string.Format( + CultureInfo.InvariantCulture, + "{0} other incident(s) still open (already reported)", + suppressedCount)) + } + }); + + if (prose is not null && + string.Equals(prose, AlertDetailText.Flatten(rendered)?.Trim(), StringComparison.Ordinal)) + { + return new Render(context, prose, 0); + } + + return new Render(rendered, prose, suppressedCount); + } + + private static bool NamesAnyOf(AlertDetailItem item, HashSet dedupKeys) + { + foreach (var (label, value) in item.Fields) + { + if (string.Equals(label, AlertIncidentRenderer.DedupKeyFactName, StringComparison.Ordinal) && + dedupKeys.Contains(value)) + { + return true; + } + } + + return false; + } +} diff --git a/PerformanceMonitor.Notifications/WebhookAlertService.cs b/PerformanceMonitor.Notifications/WebhookAlertService.cs index a3efd95eda..a7be59271c 100644 --- a/PerformanceMonitor.Notifications/WebhookAlertService.cs +++ b/PerformanceMonitor.Notifications/WebhookAlertService.cs @@ -161,7 +161,8 @@ public async Task TrySendWebhookAlertsAsync( window (a distinct fingerprint is not throttled by an unrelated prior incident); stamp every candidate key only after a successful post. Seeds the webhook last-sent time from the alert log on first touch per key (#1145), unless the store is null (no seeding). No - incidents -> the metric-level fallback key (today's behavior). */ + incidents -> the metric-level fallback key (today's behavior). WHETHER to post only; + #3313's filter below decides WHICH incidents the post contains. */ var decision = await _cooldown.EvaluateAsync( serverId, metricName, context?.Incidents, TimeSpan.FromMinutes(_settings.EmailCooldownMinutes)); @@ -173,28 +174,36 @@ the alert log on first touch per key (#1145), unless the store is null (no seedi bool sent = false; + /* #3313: the decision above says the alert posts; this says WHICH of its incidents the card + contains. Rendering all of them re-delivered fingerprints that were still inside their own + window and had gone out minutes earlier, riding along on whichever sibling was fresh. Applied + ONCE for the whole fan-out, like triageUrl below and for the same reason: four channels + describing the same firing must not disagree about what it covers. Resolves the prose too, + against the UNFILTERED context — see IncidentDeliveryFilter for why that basis is the only + correct one. #3297's null-when-redundant behaviour is unchanged. */ + var render = IncidentDeliveryFilter.ForDelivery(context, detailText, decision.DeliverableDedupKeys); + var renderContext = render.Context; + var prose = render.Prose; + /* #2710: the triage-page link, computed ONCE for the whole fan-out so all four channels carry the SAME URL for the same firing. Keyed by (server, metric, now, dedup key) rather than an alert-history id, because the history row is written AFTER delivery — the page resolves the row on read. Null (base URL unset/invalid) means every channel omits the link; delivery is never gated on it. The dedup key uses the same serverId-else-serverName identity the generic - channel's {{dedup_key}} token uses, so link, token, and PagerDuty all correlate. */ + channel's {{dedup_key}} token uses, so link, token, and PagerDuty all correlate — and it + reads the RENDERED incidents, so the anchor names an incident the card actually shows. */ var triageUrl = TriageLink.Build( _settings.TriageBaseUrl, serverName, metricName, DateTime.UtcNow, - DerivePagerDutyDedupKey(string.IsNullOrEmpty(serverId) ? serverName : serverId, metricName, context)); - - /* #3297: null when the alert carries no prose, or when its prose is only a flattening of the - structured context every channel below already renders. */ - var prose = AlertDetailText.ProseForDelivery(detailText, context); + DerivePagerDutyDedupKey(string.IsNullOrEmpty(serverId) ? serverName : serverId, metricName, renderContext)); if (TeamsConfigured) { - sent |= await TrySendTeamsAlertAsync(metricName, serverName, currentValue, thresholdValue, context, triageUrl, prose, displayName); + sent |= await TrySendTeamsAlertAsync(metricName, serverName, currentValue, thresholdValue, renderContext, triageUrl, prose, displayName); } if (SlackConfigured) { - sent |= await TrySendSlackAlertAsync(metricName, serverName, currentValue, thresholdValue, context, triageUrl, prose, displayName); + sent |= await TrySendSlackAlertAsync(metricName, serverName, currentValue, thresholdValue, renderContext, triageUrl, prose, displayName); } if (GenericConfigured) @@ -202,12 +211,12 @@ structured context every channel below already renders. */ /* Generic webhook: the payload's "metric" field is a machine key an automation correlates on, so it stays the immutable metric name — the display name is a human-title concern only, and this channel has no title. The prose detail DOES go, because it is alert content. */ - sent |= await TrySendGenericAlertAsync(metricName, serverName, currentValue, thresholdValue, serverId, context, triageUrl, prose); + sent |= await TrySendGenericAlertAsync(metricName, serverName, currentValue, thresholdValue, serverId, renderContext, triageUrl, prose); } if (PagerDutyConfigured) { - sent |= await TrySendPagerDutyAlertAsync(metricName, serverName, currentValue, thresholdValue, serverId, context, triageUrl, prose, displayName); + sent |= await TrySendPagerDutyAlertAsync(metricName, serverName, currentValue, thresholdValue, serverId, renderContext, triageUrl, prose, displayName); } if (sent) From 752b99054431bde727caf13a2afd0b8f3cc6be18 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:00:09 -0400 Subject: [PATCH 2/4] Assert both incident orders in the #3313 fixtures Both the cooldown's ShouldSend and its deliverable-key list are reductions over an ordered key list, and both fixtures put the fresh fingerprint last -- so a last-one-wins reduction was right on the one order they exercised. Measured: replacing the ShouldSend fold with a plain assignment passed all 37 checks. Each fixture now evaluates both orders, and the same mutation fails. --- .../Darling.Tests/IncidentDeliveryFilterTests.cs | 13 +++++++++++++ Lite.Tests/IncidentCooldownTests.cs | 8 ++++++++ 2 files changed, 21 insertions(+) diff --git a/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs b/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs index 6e94a0f692..fcdc711582 100644 --- a/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs +++ b/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs @@ -72,6 +72,19 @@ public void AMixedBatch_RendersOnlyTheDeliverableIncident_AndFootnotesTheRest() var footer = Assert.Single( render.Context.Details, d => d.Heading == IncidentDeliveryFilter.OtherIncidentsHeading); Assert.Contains(footer.Fields, f => f.Value == Footer); + + /* The same batch with the stale fingerprint LAST. Position is not identity, and a filter that + reduced over the ordered list rather than testing set membership would be right on exactly one + of the two orders — a mutation of the cooldown's own ordered reduction survived the single-order + form of this fixture, which is why both are asserted here too. */ + var flipped = new AlertContext(); + AlertIncidentRenderer.Apply(flipped, Incidents().Reverse().ToList()); + + var flippedRender = IncidentDeliveryFilter.ForDelivery(flipped, null, new[] { Fresh }); + + Assert.Equal(1, flippedRender.SuppressedIncidentCount); + Assert.Equal(Fresh, Assert.Single(flippedRender.Context!.Incidents!).DedupKey); + Assert.DoesNotContain(flippedRender.Context.Details, DedupKeyIs(Stale)); } /// diff --git a/Lite.Tests/IncidentCooldownTests.cs b/Lite.Tests/IncidentCooldownTests.cs index 001954f86e..bb4cdd04db 100644 --- a/Lite.Tests/IncidentCooldownTests.cs +++ b/Lite.Tests/IncidentCooldownTests.cs @@ -179,6 +179,14 @@ public async Task Decision_NamesOnlyTheFingerprintsOutsideTheirOwnWindow() Assert.True(summary.ShouldSend); Assert.Equal(2, summary.Keys.Count); // both keys are still stamped on success (#1154) Assert.Equal(new[] { "B" }, summary.DeliverableDedupKeys); + + /* And with the fresh one FIRST. Both members are reductions over an ordered key list, so a + last-one-wins or first-one-wins reduction is right on exactly one of the two orders — measured, + not theoretical: writing this assertion is what caught a mutation of the ShouldSend reduction + that the single-order form above passed. */ + var reversed = await cd.EvaluateAsync("1", "Deadlocks Detected", Incidents("B", "A"), Window); + Assert.True(reversed.ShouldSend); + Assert.Equal(new[] { "B" }, reversed.DeliverableDedupKeys); } /// From 2d694c615c8029ef0d0630c5c6effe8e19e2416d Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:02:34 -0400 Subject: [PATCH 3/4] Keep the #3297 detailText doc accurate, and drop an unread tuple element --- Darling/Darling.Tests/IncidentDeliveryFilterTests.cs | 12 ++++++------ .../WebhookAlertService.cs | 5 ++++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs b/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs index fcdc711582..73c50361ac 100644 --- a/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs +++ b/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs @@ -249,7 +249,7 @@ public async Task ASummaryBatch_DeliversOnlyTheFreshFingerprint_WhileTheHistoryR using var smtp = new CapturingSmtpEndpoint(); var history = new SeedingHistoryStore(Stale, DateTime.UtcNow - TimeSpan.FromMinutes(1)); - var (deliverer, _) = BuildDeliverer(endpoint, smtp, history); + var deliverer = BuildDeliverer(endpoint, smtp, history); var context = TwoIncidents(); await deliverer.DeliverAsync(Outcome(context), TestContext.Current.CancellationToken); @@ -287,7 +287,7 @@ public async Task ABatchWhoseEveryFingerprintIsFresh_DeliversAllOfThem() using var smtp = new CapturingSmtpEndpoint(); var history = new SeedingHistoryStore(seededDedupKey: null, DateTime.UtcNow); - var (deliverer, _) = BuildDeliverer(endpoint, smtp, history); + var deliverer = BuildDeliverer(endpoint, smtp, history); await deliverer.DeliverAsync(Outcome(TwoIncidents()), TestContext.Current.CancellationToken); @@ -323,7 +323,7 @@ public async Task AnIndependentProse_StillReachesTheWire_OnAFilteredCard() using var smtp = new CapturingSmtpEndpoint(); var history = new SeedingHistoryStore(Stale, DateTime.UtcNow - TimeSpan.FromMinutes(1)); - var (deliverer, _) = BuildDeliverer(endpoint, smtp, history); + var deliverer = BuildDeliverer(endpoint, smtp, history); var context = TwoIncidents(); await deliverer.DeliverAsync( @@ -349,7 +349,7 @@ public async Task PerEventMode_PostsTheFreshFingerprintOnly_AndRecordsBoth() using var smtp = new CapturingSmtpEndpoint(); var history = new SeedingHistoryStore(Stale, DateTime.UtcNow - TimeSpan.FromMinutes(1)); - var (deliverer, _) = BuildDeliverer( + var deliverer = BuildDeliverer( endpoint, smtp, history, config => config.Alerts.DeliveryMode = AlertNotificationMode.PerEvent); await deliverer.DeliverAsync(Outcome(TwoIncidents()), TestContext.Current.CancellationToken); @@ -392,7 +392,7 @@ ProseForDelivery recognise it as a restatement and suppress it. */ context, AlertContextBuilders.ContextToDetailText(context), NumericCurrentValue: 2, NumericThresholdValue: 1, Muted: false, Severity: null); - private static (DarlingAlertDeliverer Deliverer, DarlingAlertSettings Settings) BuildDeliverer( + private static DarlingAlertDeliverer BuildDeliverer( CapturingWebhookEndpoint endpoint, CapturingSmtpEndpoint smtp, IAlertHistoryStore history, @@ -412,7 +412,7 @@ private static (DarlingAlertDeliverer Deliverer, DarlingAlertSettings Settings) var settings = new DarlingAlertSettings(config); var webhooks = new WebhookAlertService( settings, DarlingAlertDeliverer.Branding, NullLogger.Instance, history); - return (new DarlingAlertDeliverer(settings, history, webhooks, NullLogger.Instance), settings); + return new DarlingAlertDeliverer(settings, history, webhooks, NullLogger.Instance); } private static Predicate DedupKeyIs(string dedupKey) => diff --git a/PerformanceMonitor.Notifications/WebhookAlertService.cs b/PerformanceMonitor.Notifications/WebhookAlertService.cs index a7be59271c..92a0713615 100644 --- a/PerformanceMonitor.Notifications/WebhookAlertService.cs +++ b/PerformanceMonitor.Notifications/WebhookAlertService.cs @@ -138,7 +138,10 @@ deployment that has one. */ /// /// #3297: the alert's flat prose detail. Resolved ONCE here, the same way triageUrl below is /// and for the same reason — all four channels must carry the same text for the same firing, and a - /// per-channel resolution would be four places for one of them to drift or be forgotten. + /// per-channel resolution would be four places for one of them to drift or be forgotten. #3313 moved + /// that resolution INTO , which is still this one + /// place: the prose and the rendered incident set are two halves of one answer, and the prose has to + /// be resolved against the alert's own context rather than the filtered copy. /// /// /// #3303: the human-facing name a custom rule carries, rendered in the Teams/Slack titles and the From ba5e1e039c9db1975ca2c7d7e4bd41ddf9aa6a7f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 11 Sep 2026 18:11:12 -0400 Subject: [PATCH 4/4] Make the shared endpoints file's rationale a block comment, not a stacked summary A file-level with no member of its own stacks against the first type's, which DocCommentHygieneTests.NoMemberCarriesTwoStackedSummaryBlocks forbids. Caught by the Darling PostgreSQL tests job. --- Darling/Darling.Tests/CapturingAlertEndpoints.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Darling/Darling.Tests/CapturingAlertEndpoints.cs b/Darling/Darling.Tests/CapturingAlertEndpoints.cs index 2c1e0d3751..3417ce93e4 100644 --- a/Darling/Darling.Tests/CapturingAlertEndpoints.cs +++ b/Darling/Darling.Tests/CapturingAlertEndpoints.cs @@ -19,13 +19,13 @@ namespace Darling.Tests; -/// -/// The loopback channel endpoints and alert-history doubles the delivery suites drive. Shared rather than -/// nested per test class because the SMTP one in particular is the only interception point email has short -/// of the protocol (see its own remarks), and a second copy of a protocol responder is a second thing to -/// get subtly wrong. Used by (#3169/#3297/#3303) and -/// (#3313). -/// +/* The loopback channel endpoints and alert-history doubles the delivery suites drive. Shared rather than + nested per test class because the SMTP one in particular is the only interception point email has short of + the protocol (see its own remarks), and a second copy of a protocol responder is a second thing to get + subtly wrong. Driven by AlertDeliveryChannelTests (#3169/#3297/#3303) and IncidentDeliveryFilterTests + (#3313). A block comment rather than an XML doc block: a file-level summary with no member of its own + stacks against the first type's, which DocCommentHygieneTests.NoMemberCarriesTwoStackedSummaryBlocks + forbids and which CI caught here. */ /// /// A loopback endpoint that records the bodies posted to it. /// rather than HttpListener on purpose: HttpListener wants a URL ACL on Windows, and the four