Make notification.deliver idempotent on retry - #1298
Open
johnpooch wants to merge 3 commits into
Open
Conversation
deliver is registered with retry=3 and re-read its batch by id with no status filter, so any retry re-sent every delivery in the batch, including ones already marked sent. Load only PENDING rows so a retry picks up exactly the deliveries that have not gone out yet. Deliveries whose enqueue-to-send lag exceeds DELIVER_MAX_AGE_HOURS are marked with a new expired status instead of being sent: PUSH_TTL bounds how long FCM buffers a message for an offline device, but it is computed at send time, so it does not bound the lag before the send. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTSUJGBPgDacmsi1kyi1ze
johnpooch
commented
Aug 29, 2026
johnpooch
commented
Aug 29, 2026
Modification logic belongs on the manager, so expire_stale moves out of notification.tasks and onto NotificationDeliveryManager, taking the max age from the caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTSUJGBPgDacmsi1kyi1ze
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FTSUJGBPgDacmsi1kyi1ze
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
notification.deliveris registered withretry=3and re-read its batch by id with nostatusfilter (service/notification/tasks.py:19), so any retry re-sent every delivery in the batch — including ones already markedsent. It now loads onlyPENDINGrows, so a retry picks up exactly the deliveries that have not gone out yet.It also drops deliveries that are too old to be worth sending: rows older than
DELIVER_MAX_AGE_HOURSare marked with a newexpiredstatus instead of being sent.PUSH_TTL(service/notification/utils.py:11) caps how long FCM buffers a message for an offline device, but it is computed at send time, so it does not bound the enqueue-to-send lag.Both filters are per-delivery, not per-batch: a batch that mixes sent and pending rows, or stale and fresh ones, still delivers the rows that qualify.
Closes #1281
Checklist
/review-pragainst this PR in Claude Code and addressed (or responded to) its findingsFour tests in
service/notification/tests.py::TestNotificationDelivercover the sent-skip, the partly-sent batch, the expiry, and the partly-stale batch. Full backend suite: 2309 passed, 8 skipped. No visual changes, so no screenshots./review-prhas not been run against this branch.