Dehydrate timeout_notifications nightly task#2877
Open
whabanks wants to merge 2 commits into
Open
Conversation
- Previously `timeout_notifications` fetched and fully loaded the ORM object graph for all notifications that were timed out. We returned a list of fully loaded notifications in order to queue up notification status callbacks for services who have callbacks set up. - It's unnecessary to fully load these objects into session as we only need a subset of the notification's properties to construct the API callback. - Instead of fully hydrating each notification object graph, we instead only return the subset of columns needed to construct the API callback after the notification is timed out. - This PR also fixes a bug where we were querying the notifications BEFORE timing them out, resulting in stale statuses being used when constructing the API callback
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the timeout_notifications nightly task/DAO flow to avoid hydrating full Notification ORM graphs when building delivery-status callbacks, and fixes the callback “stale status” issue by updating statuses before constructing callback payloads.
Changes:
- Refactors
_timeout_notificationsto update rows first and return lightweight callback-ready data (instead of ORMNotificationobjects). - Extracts
Notification.formatted_statuscomputation into a scalar helper (compute_formatted_status) to support dehydrated callback rows. - Updates/extends tests to validate callback payload status formatting and adjusts nightly-task callback expectations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
app/dao/notifications_dao.py |
Changes timeout logic to bulk-update then return reduced callback data instead of ORM notifications. |
app/models.py |
Adds compute_formatted_status and reuses it from Notification.formatted_status. |
app/types.py |
Introduces NotificationCallbackData dataclass for callback dispatch payload shaping. |
tests/app/dao/notification_dao/test_notification_dao.py |
Adds assertions for callback formatted_status and new edge-case coverage for formatted status computation. |
tests/app/celery/test_nightly_tasks.py |
Updates callback task argument expectations to match dehydrated return values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
-The previous approach of select-then-update could have still materialized a lot of objects in memory upon execution and introduced a potential race condition. - In the refactored approach we use an update CTE for a single atomic transaction that leverages UPDATE ... RETURNING directly via SQLAlchemy core.
whabanks
commented
May 27, 2026
| bounce_response = update.get("bounce_response") | ||
| provider_response = update.get("provider_response") | ||
| feedback_reason = update.get("feedback_reason") | ||
| for u in updates: |
Contributor
Author
There was a problem hiding this comment.
This was colliding with the update import from sqlalchemy.
whabanks
commented
May 27, 2026
| @@ -554,17 +557,63 @@ def dao_delete_notifications_by_id(notification_id): | |||
|
|
|||
|
|
|||
| def _timeout_notifications(current_statuses, new_status, timeout_start, updated_at): | |||
Contributor
Author
There was a problem hiding this comment.
We could add batching to this task in the future if we're still seeing issues with performance here.
3 tasks
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
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.
Summary | Résumé
In the same vein as 2876, this PR attempts to memory optimize
timeout_notificationsby not fully hydrating the list of notifications objects that it timed out. We fetch this list of notifications post-timeout in order to construct API callbacks for services with them enabled.What we were doing before
timeout_notificationsfetched and fully loaded the ORM object graph for all notifications that were timed out. We returned a list of fully loaded notifications in order to queue up notification status callbacks for services who have callbacks set up.Why it's not great & the change
It's unnecessary to fully load these objects into session as we only need a subset of the notification's properties to construct the API callback. Instead of fully hydrating each notification object graph, we instead only return the subset of columns needed to construct the API callback after the notification is timed out.
Secondary bug fix
This PR also fixes a bug where we were querying the notifications BEFORE timing them out, resulting in stale statuses being used when constructing the API callback
Related Issues | Cartes liées
Test instructions | Instructions pour tester la modification
Release Instructions | Instructions pour le déploiement
None.
Reviewer checklist | Liste de vérification du réviseur