Skip to content

Commit

Permalink
update
Browse files Browse the repository at this point in the history
  • Loading branch information
Chris Johnson committed Jan 17, 2025
1 parent 7fbd4fc commit 8ec8779
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 46 deletions.
45 changes: 16 additions & 29 deletions app/celery/process_delivery_status_result_tasks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -306,16 +306,6 @@ def can_retry_sms_request(
# Calculate the time elapsed since the message was sent
time_elapsed = datetime.datetime.utcnow() - sent_at

current_app.logger.info(
'Retry SMS conditional | retry_attempts: %s | max_retries: %s | sent_at: %s | elapsed: %s | is_retryable_count %s | is_retryable_window %s',
str(retries),
str(max_retries),
str(sent_at),
str(time_elapsed),
str(retries < max_retries),
str(time_elapsed < retry_window),
)

return (retries <= max_retries) and (time_elapsed < retry_window)


Expand All @@ -342,8 +332,8 @@ def get_sms_retry_delay(retry_count: int) -> int:
)

# Safeguard against retry counts outside the defined range
retry_count = max(retry_count, 0)
base_delay, jitter_range = delay_with_jitter[min(retry_count, len(delay_with_jitter) - 1)]
index = max(retry_count - 1, 0)
base_delay, jitter_range = delay_with_jitter[min(index, len(delay_with_jitter) - 1)]

# Apply jitter
delay = int(base_delay + random.randint(-jitter_range, jitter_range)) # nosec non-cryptographic use case
Expand Down Expand Up @@ -378,7 +368,7 @@ def sms_attempt_retry(
sms_status: SmsStatusRecord,
event_timestamp: str | None = None,
event_in_seconds: int = 300, # Don't retry by default
) -> None:
) -> tuple[Notification, SmsStatusRecord]:
"""Attempt retry sending notification.
Retry notification if within permissible limits and update notification.
Expand Down Expand Up @@ -423,20 +413,6 @@ def sms_attempt_retry(
raise NonRetryableException('Unable to update SMS retry count')

if can_retry_sms_request(retry_count, CARRIER_SMS_MAX_RETRIES, notification.sent_at, CARRIER_SMS_MAX_RETRY_WINDOW):
try:
notification: Notification = dao_update_sms_notification_delivery_status(
notification_id=notification.id,
notification_type=notification.notification_type,
new_status=notification.status,
new_status_reason=notification.status_reason,
segments_count=sms_status.message_parts,
cost_in_millicents=sms_status.price_millicents + notification.cost_in_millicents,
)
statsd_client.incr(f'clients.sms.{sms_status.provider}.delivery.status.{sms_status.status}')
except Exception:
statsd_client.incr(f'clients.sms.{sms_status.provider}.status_update.error')
raise NonRetryableException('Unable to update notification')

retry_delay = get_sms_retry_delay(retry_count)

current_app.logger.info(
Expand All @@ -448,7 +424,18 @@ def sms_attempt_retry(
str(retry_count),
)

send_notification_to_queue(notification, notification.service.research_mode, delay=retry_delay)
send_notification_to_queue(
notification,
notification.service.research_mode,
recipient_id_type=notification.recipient_identifiers,
sms_sender_id=notification.sms_sender_id,
delay=retry_delay,
)

# notification has been re-queued for delivery
# overwrite sms_status current notification status so client update not triggered in sms_status_update
sms_status.status = notification.status
sms_status.status_reason = notification.status_reason

current_app.logger.info(
'Retry attempted %s logic | reference: %s | notification_id: %s | notification_status: %s | notification_status_reason: %s | sms_status: %s | sms_status_reason: %s | retry_count: %s',
Expand All @@ -462,7 +449,7 @@ def sms_attempt_retry(
str(retry_count),
)
else:
# mark as permanant failure
# mark as permanant failure so client can be updated
sms_status.status = NOTIFICATION_PERMANENT_FAILURE
sms_status.status_reason = STATUS_REASON_UNDELIVERABLE

Expand Down
33 changes: 16 additions & 17 deletions tests/app/celery/test_process_delivery_status_result_tasks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
_get_provider_info,
can_retry_sms_request,
get_notification_platform_status,
get_sms_retry_delay,
process_delivery_status,
sms_attempt_retry,
sms_status_update,
Expand Down Expand Up @@ -564,6 +565,21 @@ def test_can_retry_exceeding_retry_window_is_false():
assert not can_retry_sms_request(retries, CARRIER_SMS_MAX_RETRIES, sent_at, CARRIER_SMS_MAX_RETRY_WINDOW)


@pytest.mark.parametrize(
'retry_count, expected_base_delay',
[
(-1, 60),
(0, 60),
(1, 60),
(2, 600),
(3, 600),
],
)
def test_get_sms_retry_delay_returns_within_delay_range(retry_count, expected_base_delay):
# delay should be within +/- 10% of expected base delay
assert int(0.9 * expected_base_delay) <= get_sms_retry_delay(retry_count) <= int(expected_base_delay * 1.1)


def test_sms_attempt_retry_not_requeued_if_retry_limit_exceeded(mocker, sample_notification):
notification = sample_notification()
sms_status = SmsStatusRecord(
Expand Down Expand Up @@ -603,20 +619,3 @@ def test_sms_attempt_retry_redis_update_exception(mocker, sample_notification):
with pytest.raises(NonRetryableException) as exc_info:
sms_attempt_retry(sms_status)
assert 'Unable to update SMS retry count' in str(exc_info)


def test_sms_attempt_retry_notification_not_found(mocker, sample_notification):
mocker.patch('app.celery.process_delivery_status_result_tasks.update_sms_retry_count', return_value=1)
mocker.patch('app.celery.process_delivery_status_result_tasks.can_retry_sms_request', return_value=True)
mocker.patch(
'app.celery.process_delivery_status_result_tasks.dao_update_sms_notification_delivery_status',
side_effect=Exception,
)

notification = sample_notification()
sms_status = SmsStatusRecord(
None, notification.reference, NOTIFICATION_TEMPORARY_FAILURE, STATUS_REASON_RETRYABLE, PINPOINT_PROVIDER
)
with pytest.raises(NonRetryableException) as exc_info:
sms_attempt_retry(sms_status)
assert 'Unable to update notification' in str(exc_info)

0 comments on commit 8ec8779

Please sign in to comment.