fix(notifications): SMS adapter short-circuits on first transient — kills duplicate amplification and bounds the dispatch sweep
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
This commit is contained in:
@@ -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:
|
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.
|
- **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.
|
- **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).
|
- **All permanent / no recipients / no SMS config / list-not-found** → `Permanent` (Park).
|
||||||
|
|
||||||
|
|||||||
+22
-10
@@ -155,17 +155,22 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte
|
|||||||
httpClient, requestUri, authHeader, smsConfig, hasMessagingService,
|
httpClient, requestUri, authHeader, smsConfig, hasMessagingService,
|
||||||
phone, body, timeoutSeconds, redactSecret, cancellationToken);
|
phone, body, timeoutSeconds, redactSecret, cancellationToken);
|
||||||
|
|
||||||
switch (attempt.Class)
|
if (attempt.Class == SmsErrorClass.Transient)
|
||||||
{
|
{
|
||||||
case SmsErrorClass.Transient:
|
// Every recipient accepted after the first transient is a guaranteed
|
||||||
firstTransientError ??= attempt.Detail;
|
// duplicate on retry (the whole notification re-sends); stopping here
|
||||||
break;
|
// also bounds a black-holed endpoint to one timeout per sweep.
|
||||||
case SmsErrorClass.Permanent:
|
firstTransientError = attempt.Detail;
|
||||||
permanentlyFailed.Add(phone);
|
break;
|
||||||
break;
|
}
|
||||||
default:
|
|
||||||
accepted.Add(phone);
|
if (attempt.Class == SmsErrorClass.Permanent)
|
||||||
break;
|
{
|
||||||
|
permanentlyFailed.Add(phone);
|
||||||
|
}
|
||||||
|
else
|
||||||
|
{
|
||||||
|
accepted.Add(phone);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -284,6 +289,13 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte
|
|||||||
/// any transient → Transient (whole notification retries); else any
|
/// any transient → Transient (whole notification retries); else any
|
||||||
/// accepted → Success (the permanently-failed numbers are noted, NOT parked); else
|
/// accepted → Success (the permanently-failed numbers are noted, NOT parked); else
|
||||||
/// (all permanent / zero accepted) → Permanent.
|
/// (all permanent / zero accepted) → Permanent.
|
||||||
|
/// <para>
|
||||||
|
/// 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
|
||||||
|
/// <paramref name="accepted"/> and <paramref name="permanentlyFailed"/> only ever
|
||||||
|
/// reflect recipients attempted before that first transient.
|
||||||
|
/// </para>
|
||||||
/// </summary>
|
/// </summary>
|
||||||
private DeliveryOutcome RollUp(
|
private DeliveryOutcome RollUp(
|
||||||
string listName,
|
string listName,
|
||||||
|
|||||||
+18
@@ -299,6 +299,24 @@ public class SmsNotificationDeliveryAdapterTests
|
|||||||
Assert.Equal(DeliveryResult.TransientFailure, outcome.Result);
|
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]
|
[Fact]
|
||||||
public async Task Deliver_ListNotFound_ReturnsPermanent()
|
public async Task Deliver_ListNotFound_ReturnsPermanent()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user