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..3417ce93e4 --- /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. 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 +/// 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..73c50361ac --- /dev/null +++ b/Darling/Darling.Tests/IncidentDeliveryFilterTests.cs @@ -0,0 +1,463 @@ +/* + * 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 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)); + } + + /// + /// 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 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); + } + + 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..bb4cdd04db 100644 --- a/Lite.Tests/IncidentCooldownTests.cs +++ b/Lite.Tests/IncidentCooldownTests.cs @@ -151,6 +151,78 @@ 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); + + /* 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); + } + + /// + /// 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..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 @@ -161,7 +164,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 +177,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 +214,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)