From 17716e7b29ff888a2608e87add5ba5759245049a Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Sat, 11 Jul 2026 16:16:33 -0300 Subject: [PATCH 1/2] fix(mailing): fail loudly when SendGrid rejects a message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SendGridMailService discarded the Response from SendEmailAsync, and the client is built with HttpErrorAsException=false, so a non-2xx reply (invalid API key, rejected recipient, rate limit) returned normally instead of throwing. Every caller — login links, webhooks, notifications — believed the mail was delivered. Inspect the status and throw on a non-success reply so the failure reaches the global error boundary instead of being swallowed. --- .../Mailing/Services/SendGridMailService.cs | 17 ++++- .../Mailing/SendGridMailServiceTests.cs | 63 +++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 src/Tests/Framework.Tests/Mailing/SendGridMailServiceTests.cs diff --git a/src/BuildingBlocks/Mailing/Services/SendGridMailService.cs b/src/BuildingBlocks/Mailing/Services/SendGridMailService.cs index b25ddd1317..615710c9f7 100644 --- a/src/BuildingBlocks/Mailing/Services/SendGridMailService.cs +++ b/src/BuildingBlocks/Mailing/Services/SendGridMailService.cs @@ -40,9 +40,24 @@ public async Task SendAsync(MailRequest request, CancellationToken ct) ConfigureRecipients(msg, request); AddAttachments(msg, request); - await _client.SendEmailAsync(msg, ct).ConfigureAwait(false); + var response = await _client.SendEmailAsync(msg, ct).ConfigureAwait(false); + + // The client is built with HttpErrorAsException=false, so a non-2xx reply (bad key, rejected + // recipient, rate limit) comes back as a Response instead of throwing. Surface it — a silently + // discarded failure makes the caller believe the mail was delivered. + if (!IsSuccess(response.StatusCode)) + { + var body = response.Body is not null + ? await response.Body.ReadAsStringAsync(ct).ConfigureAwait(false) + : string.Empty; + throw new InvalidOperationException( + $"SendGrid rejected the message with status {(int)response.StatusCode}. {body}".TrimEnd()); + } } + private static bool IsSuccess(System.Net.HttpStatusCode statusCode) => + (int)statusCode is >= 200 and < 300; + private void ValidateConfiguration() { if (_settings.SendGrid?.ApiKey is null) diff --git a/src/Tests/Framework.Tests/Mailing/SendGridMailServiceTests.cs b/src/Tests/Framework.Tests/Mailing/SendGridMailServiceTests.cs new file mode 100644 index 0000000000..3c632a0094 --- /dev/null +++ b/src/Tests/Framework.Tests/Mailing/SendGridMailServiceTests.cs @@ -0,0 +1,63 @@ +using System.Collections.ObjectModel; +using System.Net; +using FSH.Framework.Mailing; +using FSH.Framework.Mailing.Services; +using Microsoft.Extensions.Options; +using NSubstitute; +using SendGrid; +using SendGrid.Helpers.Mail; + +namespace Framework.Tests.Mailing; + +// REL-01 repro: SendGridMailService discards the Response from SendEmailAsync and the client is +// built with HttpErrorAsException=false, so a non-2xx SendGrid reply is swallowed and the caller +// believes the mail was delivered. This test pins the DESIRED behaviour (a non-success status must +// surface) and is expected to FAIL against the current code — that failure IS the confirmation. +public sealed class SendGridMailServiceTests +{ + private static SendGridMailService BuildService(ISendGridClient client) + { + var options = Options.Create(new MailOptions + { + UseSendGrid = true, + From = "noreply@x.com", + SendGrid = new SendGridOptions { ApiKey = "sg-key", From = "noreply@x.com" }, + }); + return new SendGridMailService(options, client); + } + + private static MailRequest ValidRequest() => + new(to: ["dest@x.com"], subject: "hi", body: "body"); + + [Fact] + public async Task SendAsync_When_SendGridReturnsNonSuccess_Should_SurfaceFailure() + { + // Arrange + var client = Substitute.For(); + client.SendEmailAsync(Arg.Any(), Arg.Any()) + .Returns(new Response(HttpStatusCode.Unauthorized, null, null)); + var service = BuildService(client); + + // Act + var send = async () => await service.SendAsync(ValidRequest(), CancellationToken.None); + + // Assert — a 401 from SendGrid must NOT be reported as a successful send. + await send.ShouldThrowAsync(); + } + + [Fact] + public async Task SendAsync_When_SendGridReturnsAccepted_Should_Complete() + { + // Arrange + var client = Substitute.For(); + client.SendEmailAsync(Arg.Any(), Arg.Any()) + .Returns(new Response(HttpStatusCode.Accepted, null, null)); + var service = BuildService(client); + + // Act + var send = async () => await service.SendAsync(ValidRequest(), CancellationToken.None); + + // Assert + await send.ShouldNotThrowAsync(); + } +} From a1751c9f03d53486309358d09eb5447596ce6147 Mon Sep 17 00:00:00 2001 From: iammukeshm Date: Mon, 13 Jul 2026 19:37:26 +0530 Subject: [PATCH 2/2] fix(mailing): only retry transient SendGrid failures, log permanent ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Throwing on every non-2xx reply routed permanent failures (bad API key, rejected recipient) back through Hangfire's automatic retry (~10 attempts), flooding the dead-letter queue with sends that can never succeed. Retry only 429/5xx (transient); log permanent 4xx at error and return. Still never silently swallows a failure — the original REL-01 concern. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BuildingBlocks/Mailing/Extensions.cs | 3 +- .../Mailing/Services/SendGridMailService.cs | 37 +++++++++--- .../Mailing/SendGridMailServiceTests.cs | 58 ++++++++++++------- 3 files changed, 67 insertions(+), 31 deletions(-) diff --git a/src/BuildingBlocks/Mailing/Extensions.cs b/src/BuildingBlocks/Mailing/Extensions.cs index fc1625eda4..1915916501 100644 --- a/src/BuildingBlocks/Mailing/Extensions.cs +++ b/src/BuildingBlocks/Mailing/Extensions.cs @@ -28,7 +28,8 @@ public static IServiceCollection AddHeroMailing(this IServiceCollection services { return new SendGridMailService( sp.GetRequiredService>(), - sp.GetRequiredService()); + sp.GetRequiredService(), + sp.GetRequiredService>()); } return new SmtpMailService(sp.GetRequiredService>(), sp.GetRequiredService>()); }); diff --git a/src/BuildingBlocks/Mailing/Services/SendGridMailService.cs b/src/BuildingBlocks/Mailing/Services/SendGridMailService.cs index 615710c9f7..96334b4124 100644 --- a/src/BuildingBlocks/Mailing/Services/SendGridMailService.cs +++ b/src/BuildingBlocks/Mailing/Services/SendGridMailService.cs @@ -1,3 +1,4 @@ +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using SendGrid; using SendGrid.Helpers.Mail; @@ -10,13 +11,16 @@ public sealed class SendGridMailService : IMailService { private readonly MailOptions _settings; private readonly ISendGridClient _client; + private readonly ILogger _logger; - public SendGridMailService(IOptions settings, ISendGridClient client) + public SendGridMailService(IOptions settings, ISendGridClient client, ILogger logger) { ArgumentNullException.ThrowIfNull(settings); ArgumentNullException.ThrowIfNull(client); + ArgumentNullException.ThrowIfNull(logger); _settings = settings.Value; _client = client; + _logger = logger; } public async Task SendAsync(MailRequest request, CancellationToken ct) @@ -43,16 +47,33 @@ public async Task SendAsync(MailRequest request, CancellationToken ct) var response = await _client.SendEmailAsync(msg, ct).ConfigureAwait(false); // The client is built with HttpErrorAsException=false, so a non-2xx reply (bad key, rejected - // recipient, rate limit) comes back as a Response instead of throwing. Surface it — a silently - // discarded failure makes the caller believe the mail was delivered. - if (!IsSuccess(response.StatusCode)) + // recipient, rate limit) comes back as a Response instead of throwing. A silently discarded + // failure makes the caller believe the mail was delivered, so it must never be ignored. + if (IsSuccess(response.StatusCode)) + { + return; + } + + var status = (int)response.StatusCode; + var body = response.Body is not null + ? await response.Body.ReadAsStringAsync(ct).ConfigureAwait(false) + : string.Empty; + + // Only retry failures that can plausibly succeed later — 429 (rate limited) and 5xx + // (SendGrid-side). Throwing routes these back through the caller's Hangfire automatic retry. + // A permanent 4xx (bad API key, rejected recipient) will never succeed on retry, so log it + // loudly and return instead of throwing — otherwise every enqueued send retries ~10x and + // floods the dead-letter queue with attempts that cannot succeed. + if (status == 429 || status >= 500) { - var body = response.Body is not null - ? await response.Body.ReadAsStringAsync(ct).ConfigureAwait(false) - : string.Empty; throw new InvalidOperationException( - $"SendGrid rejected the message with status {(int)response.StatusCode}. {body}".TrimEnd()); + $"SendGrid transiently failed the message with status {status}. {body}".TrimEnd()); } + + _logger.LogError( + "SendGrid permanently rejected the message with status {StatusCode}. {Body}", + status, + body); } private static bool IsSuccess(System.Net.HttpStatusCode statusCode) => diff --git a/src/Tests/Framework.Tests/Mailing/SendGridMailServiceTests.cs b/src/Tests/Framework.Tests/Mailing/SendGridMailServiceTests.cs index 3c632a0094..87150fec8b 100644 --- a/src/Tests/Framework.Tests/Mailing/SendGridMailServiceTests.cs +++ b/src/Tests/Framework.Tests/Mailing/SendGridMailServiceTests.cs @@ -1,7 +1,7 @@ -using System.Collections.ObjectModel; using System.Net; using FSH.Framework.Mailing; using FSH.Framework.Mailing.Services; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; using NSubstitute; using SendGrid; @@ -9,10 +9,10 @@ namespace Framework.Tests.Mailing; -// REL-01 repro: SendGridMailService discards the Response from SendEmailAsync and the client is -// built with HttpErrorAsException=false, so a non-2xx SendGrid reply is swallowed and the caller -// believes the mail was delivered. This test pins the DESIRED behaviour (a non-success status must -// surface) and is expected to FAIL against the current code — that failure IS the confirmation. +// REL-01: SendGridMailService must not swallow a non-2xx SendGrid reply (the client is built with +// HttpErrorAsException=false, so a failure comes back as a Response, not an exception). Transient +// failures (429/5xx) throw so the caller's Hangfire job retries; permanent failures (other 4xx) are +// logged and returned — retrying a bad key / rejected recipient only floods the dead-letter queue. public sealed class SendGridMailServiceTests { private static SendGridMailService BuildService(ISendGridClient client) @@ -23,41 +23,55 @@ private static SendGridMailService BuildService(ISendGridClient client) From = "noreply@x.com", SendGrid = new SendGridOptions { ApiKey = "sg-key", From = "noreply@x.com" }, }); - return new SendGridMailService(options, client); + return new SendGridMailService(options, client, NullLogger.Instance); + } + + private static ISendGridClient ClientReturning(HttpStatusCode status) + { + var client = Substitute.For(); + client.SendEmailAsync(Arg.Any(), Arg.Any()) + .Returns(new Response(status, null, null)); + return client; } private static MailRequest ValidRequest() => new(to: ["dest@x.com"], subject: "hi", body: "body"); - [Fact] - public async Task SendAsync_When_SendGridReturnsNonSuccess_Should_SurfaceFailure() + [Theory] + [InlineData(HttpStatusCode.TooManyRequests)] // 429 — rate limited + [InlineData(HttpStatusCode.InternalServerError)] // 500 — SendGrid-side + [InlineData(HttpStatusCode.ServiceUnavailable)] // 503 — SendGrid-side + public async Task SendAsync_When_TransientFailure_Should_Throw_ForRetry(HttpStatusCode status) { - // Arrange - var client = Substitute.For(); - client.SendEmailAsync(Arg.Any(), Arg.Any()) - .Returns(new Response(HttpStatusCode.Unauthorized, null, null)); - var service = BuildService(client); + var service = BuildService(ClientReturning(status)); - // Act var send = async () => await service.SendAsync(ValidRequest(), CancellationToken.None); - // Assert — a 401 from SendGrid must NOT be reported as a successful send. + // Throwing routes the send back through the caller's Hangfire automatic retry. await send.ShouldThrowAsync(); } + [Theory] + [InlineData(HttpStatusCode.Unauthorized)] // 401 — bad API key + [InlineData(HttpStatusCode.BadRequest)] // 400 — rejected recipient / malformed + [InlineData(HttpStatusCode.Forbidden)] // 403 — sender not verified + public async Task SendAsync_When_PermanentRejection_Should_NotThrow_ToAvoidRetryStorm(HttpStatusCode status) + { + var service = BuildService(ClientReturning(status)); + + var send = async () => await service.SendAsync(ValidRequest(), CancellationToken.None); + + // Logged as an error (surfaced to ops) but not thrown — a retry cannot make it succeed. + await send.ShouldNotThrowAsync(); + } + [Fact] public async Task SendAsync_When_SendGridReturnsAccepted_Should_Complete() { - // Arrange - var client = Substitute.For(); - client.SendEmailAsync(Arg.Any(), Arg.Any()) - .Returns(new Response(HttpStatusCode.Accepted, null, null)); - var service = BuildService(client); + var service = BuildService(ClientReturning(HttpStatusCode.Accepted)); - // Act var send = async () => await service.SendAsync(ValidRequest(), CancellationToken.None); - // Assert await send.ShouldNotThrowAsync(); } }