From 839baa7880133580f4f25b4dc22497b9499d99d1 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 10 Jul 2026 03:49:18 -0400 Subject: [PATCH] =?UTF-8?q?fix(notifications):=20SMS=20adapter=20short-cir?= =?UTF-8?q?cuits=20on=20first=20transient=20=E2=80=94=20kills=20duplicate?= =?UTF-8?q?=20amplification=20and=20bounds=20the=20dispatch=20sweep?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj --- .../Component-NotificationService.md | 2 +- .../SmsNotificationDeliveryAdapter.cs | 32 +++++++++++++------ .../SmsNotificationDeliveryAdapterTests.cs | 18 +++++++++++ 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/docs/requirements/Component-NotificationService.md b/docs/requirements/Component-NotificationService.md index 1dbf33ff..0ae0fc12 100644 --- a/docs/requirements/Component-NotificationService.md +++ b/docs/requirements/Component-NotificationService.md @@ -99,7 +99,7 @@ The `SmsNotificationDeliveryAdapter` delivers notifications to an SMS list via t SMS has no BCC mechanism. The adapter sends one Twilio request per recipient and classifies each: - **All accepted** → `Success`; `ResolvedTargets` is snapshotted with the accepted numbers. -- **Any transient failure** → `Transient` (the whole notification retries at the fixed interval, then Parks after max-retries). Numbers already accepted on a prior attempt are re-texted on retry — the same "re-send to all" characteristic the Email adapter already has with BCC. (v1 does not track per-recipient state; that is a documented future enhancement.) +- **Any transient failure** → `Transient` (the whole notification retries at the fixed interval, then Parks after max-retries). Numbers already accepted on a prior attempt are re-texted on retry — the same "re-send to all" characteristic the Email adapter already has with BCC. The recipient loop stops at the **first** transient rather than attempting the remaining recipients: every recipient accepted after the first transient would just be a guaranteed duplicate text on retry, and stopping early also bounds a black-holed endpoint to one request timeout per sweep instead of recipients × timeout. (v1 does not track per-recipient state; that is a documented future enhancement.) - **No transient failures, mix of accepted + permanent-bad** → `Success` to the good numbers; permanently-bad numbers are recorded in `LastError`. The notification is not parked if anything got through. - **All permanent / no recipients / no SMS config / list-not-found** → `Permanent` (Park). diff --git a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs index 642849e1..7c0c3037 100644 --- a/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.NotificationOutbox/Delivery/SmsNotificationDeliveryAdapter.cs @@ -155,17 +155,22 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte httpClient, requestUri, authHeader, smsConfig, hasMessagingService, phone, body, timeoutSeconds, redactSecret, cancellationToken); - switch (attempt.Class) + if (attempt.Class == SmsErrorClass.Transient) { - case SmsErrorClass.Transient: - firstTransientError ??= attempt.Detail; - break; - case SmsErrorClass.Permanent: - permanentlyFailed.Add(phone); - break; - default: - accepted.Add(phone); - break; + // Every recipient accepted after the first transient is a guaranteed + // duplicate on retry (the whole notification re-sends); stopping here + // also bounds a black-holed endpoint to one timeout per sweep. + firstTransientError = attempt.Detail; + break; + } + + if (attempt.Class == SmsErrorClass.Permanent) + { + permanentlyFailed.Add(phone); + } + else + { + accepted.Add(phone); } } @@ -284,6 +289,13 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte /// any transient → Transient (whole notification retries); else any /// accepted → Success (the permanently-failed numbers are noted, NOT parked); else /// (all permanent / zero accepted) → Permanent. + /// + /// The caller's recipient loop stops at the FIRST transient rather than attempting + /// every recipient: every recipient accepted after the first transient is a + /// guaranteed duplicate on retry (the whole notification re-sends), so + /// and only ever + /// reflect recipients attempted before that first transient. + /// /// private DeliveryOutcome RollUp( string listName, diff --git a/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/SmsNotificationDeliveryAdapterTests.cs b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/SmsNotificationDeliveryAdapterTests.cs index 52128eb2..538d8ad7 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/SmsNotificationDeliveryAdapterTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.NotificationOutbox.Tests/Delivery/SmsNotificationDeliveryAdapterTests.cs @@ -299,6 +299,24 @@ public class SmsNotificationDeliveryAdapterTests Assert.Equal(DeliveryResult.TransientFailure, outcome.Result); } + [Fact] + public async Task MidListTransient_ShortCircuits_RemainingRecipientsNotAttempted() + { + // Three recipients; the FIRST one comes back transient (500). Every recipient + // accepted after a transient is a guaranteed duplicate on retry (the whole + // notification re-sends), so the adapter must stop at the first transient + // instead of attempting the remaining two. + SetupList(phones: new[] { "+15551112222", "+15553334444", "+15555556666" }); + var handler = ScriptedHttpMessageHandler.ForStatuses( + HttpStatusCode.InternalServerError, HttpStatusCode.Created, HttpStatusCode.Created); + var adapter = CreateAdapter(handler); + + var outcome = await adapter.DeliverAsync(MakeNotification()); + + Assert.Equal(DeliveryResult.TransientFailure, outcome.Result); + Assert.Single(handler.Requests); + } + [Fact] public async Task Deliver_ListNotFound_ReturnsPermanent() {