Fix the API bulk notifications priority being ignored

[jimleroyer]

Summary | Résumé

Fixes a bug in the batch-save path that caused priority notifications to be routed to the wrong Celery queue. In try_to_send_notifications_to_queue, the template loop variable leaked out of the batch loop, so every notification in a mixed-priority batch was routed as if it had the last notification's priority. A priority email batched with a normal email would land on send-email-medium and warn queue_name send-email-high but was sent to queue send-email-medium.

Each notification is now routed by its own queue_name (correctly set per-notification in persist_notifications), so mixed-priority batches route correctly. The queue-override map has been left keyed by strings, matching the in-session type of notification.id inside these functions; a defensive str() cast at the lookup site makes the code robust if a future refactor causes .id to be exposed as a uuid.UUID.

Related Issues | Cartes liées

• Investigate 5 minutes delay for failed high priority notifications

What changed

app/celery/tasks.py

• Per-notification routing in try_to_send_notifications_to_queue: fall back to notification.queue_name before the stale batch template, so mixed-priority batches route correctly. Added a TODO noting that the override map can be removed once persist_notifications handles the CSV bulk-redirect rule.
• Defensive str() cast on the map lookup: notification_id_queue.get(str(notification_obj.id)). Inside save_smss/save_emails, notification.id is a string in-session (SQLAlchemy's bulk_save_objects doesn't refresh objects), so the lookup works as before — but the cast keeps it robust if a future change ever refreshes the object and turns .id into a uuid.UUID. Added a short comment explaining the ambiguity, plus a Dict[str, Optional[str]] type hint on the map for clarity.

tests/app/celery/test_tasks.py

New TestTryToSendNotificationsToQueue class with 5 tests:

• test_uses_each_notifications_own_queue_name_in_mixed_priority_batch — mixed batch routes each notification to its own queue.
• test_notification_id_queue_override_wins_over_queue_name — CSV bulk-redirect override takes precedence when set.
• test_falls_back_to_template_queue_when_queue_name_and_map_are_both_empty — legacy fallback path.
• test_priority_notification_not_routed_to_last_batch_notifications_queue — direct regression for the production bug (mirrors the 5-notification batch from the log).
• test_lookup_is_robust_to_notification_id_being_a_uuid_or_string — documents the defensive str() cast so the lookup keeps working whether notification.id is a string (current behavior) or a uuid.UUID (future).

Also updated one existing TestSaveEmails test expectation to reflect the new (correct) behavior where a mocked choose_queue return value now actually flows through to the delivery call.

Impact / risk

• What was broken: in mixed-priority batches, priority notifications occasionally landed on the medium queue, defeating the intent of the priority lane.
• User-visible effect: high-priority messages could sit behind normal traffic on the medium queue, showing up as extra latency (and, when delivery failed, as a 5-minute retry delay instead of 25 seconds — see companion PR on the retry path).
• After this PR: each notification is routed by its own priority; SMS and email paths behave consistently; CSV bulk-redirect and other pre-existing map overrides continue to work as before.

Test instructions | Instructions pour tester la modification

1. Run the new regression tests:

   pytest tests/app/celery/test_tasks.py::TestTryToSendNotificationsToQueue -v
   

   All 5 tests should pass.

2. Run the broader batch-save tests to confirm no regressions:

   pytest tests/app/celery/test_tasks.py::TestBatchSaving tests/app/celery/test_tasks.py::TestSaveEmails tests/app/celery/test_tasks.py::TestSaveSmss -v
   

3. After deploying to staging, verify with CloudWatch Logs Insights that the has queue_name send-email-high but was sent to queue send-email-medium warning no longer appears for mixed-priority batches:

   fields @timestamp, @message
   | filter @message like /has queue_name send-email-high but was sent to queue/
   | sort @timestamp desc
   | limit 20
   

Release Instructions | Instructions pour le déploiement

None.

Reviewer checklist | Liste de vérification du réviseur

• This PR does not break existing functionality.
• This PR does not violate GCNotify's privacy policies.
• This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive.
• This PR does not significantly alter performance.
• Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).

⚠ If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected.

[Copilot]

Pull request overview

Fixes incorrect Celery queue routing for bulk-saved notifications where a stale template variable could cause mixed-priority batches to be sent to the wrong email queue (ignoring per-notification priority).

Changes:

• Update try_to_send_notifications_to_queue to prefer each notification’s own queue_name before falling back to the batch-level template queue.
• Add targeted regression tests covering mixed-priority batches, override precedence, and legacy fallback behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
app/celery/tasks.py	Adjusts queue resolution logic in try_to_send_notifications_to_queue to avoid stale-template routing and preserve per-notification queueing.
tests/app/celery/test_tasks.py	Adds a new regression test suite validating correct per-notification queue routing and override/fallback behavior.

Suppressed comments (3)

tests/app/celery/test_tasks.py:565

• Same key-type issue as above: notification_id_queue should reflect the real payload shape (string IDs), otherwise this test may pass even if the override lookup fails in production.

        saved_notifications = [self._make_notification(notification_id, QueueNames.SEND_EMAIL_HIGH)]
        notification_id_queue = {notification_id: QueueNames.SEND_EMAIL_LOW}

tests/app/celery/test_tasks.py:583

• Same key-type issue as above: notification_id_queue is typically keyed by string IDs, so the fallback-path test should use str(notification_id) as the key to match production behavior.

        saved_notifications = [self._make_notification(notification_id, None)]
        notification_id_queue = {notification_id: None}

tests/app/celery/test_tasks.py:609

• Same key-type issue as above: use string IDs for notification_id_queue to match the signed payload shape, otherwise this regression test may not reflect the production behavior.

            self._make_notification(normal_id, QueueNames.SEND_EMAIL_MEDIUM),
        ]
        notification_id_queue = {n.id: None for n in saved_notifications}

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jimleroyer]

This is the test that targets the new bug that was found by the AI reviewer in this PR. Different from the original bug that this PR also targets (i.e. missing/lost priority type for bulk API notifications).

[jimleroyer]

Bug fix #1

[jimleroyer]

Bug fix #2

[andrewleith]

This seems reasonable to me for email. Any reason this fix should not also be applied to the SMS code? Doesn't that have the same issue?

[jimleroyer]

@andrewleith Hmm I was under the impression that the try_to_send_notifications_to_queue function would cover both paths, but it seems it's only used for emails. I will add tests around SMS path first, to add necessary coverage, and see if we can reuse try_to_send_notifications_to_queue in there too.