fix(scheduler): isolate per-subscriber DB commit to prevent duplicate emails (#1118)#1134
Open
Twenitrix wants to merge 3 commits into
Open
fix(scheduler): isolate per-subscriber DB commit to prevent duplicate emails (#1118)#1134Twenitrix wants to merge 3 commits into
Twenitrix wants to merge 3 commits into
Conversation
… emails on failure (imDarshanGK#1118) - Move db.commit() inside the loop immediately after each successful send - Wrap each subscriber in its own try/except with db.rollback() on failure - One subscriber failing no longer aborts the rest of the batch - Added failed counter and improved run summary log line - Add 2 regression tests: partial SMTP failure isolation and stats exception isolation (order-independent)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a reliability bug in the weekly Sunday digest scheduler by preventing a mid-batch failure from rolling back last_sent_at updates for subscribers whose emails were already sent, which previously risked duplicate digests on the next run.
Changes:
- Move the database commit for
last_sent_atupdates to occur per subscriber after a successful send. - Add per-subscriber exception isolation with rollback + logging so one bad subscriber doesn’t abort the whole batch.
- Add regression tests that simulate mid-loop failures (SMTP error / stats exception) and assert partial progress is committed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/app/services/scheduler.py | Commits last_sent_at per subscriber and isolates failures to prevent batch-wide rollback and duplicate emails. |
| backend/tests/test_digest.py | Adds regression tests validating partial commit behavior when one subscriber fails mid-run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+38
to
+42
| for sub in subs: | ||
| stats = compute_subscriber_stats(db, sub.email) | ||
| if not stats: | ||
| log.debug("No stats for %s, skipping", sub.email) | ||
| continue | ||
| ok = send_digest(stats, sub.unsubscribe_token) | ||
| if ok: | ||
| sub.last_sent_at = datetime.now(UTC) | ||
| sent += 1 | ||
| else: | ||
| log.warning("Failed to deliver digest to %s", sub.email) | ||
| try: | ||
| stats = compute_subscriber_stats(db, sub.email) | ||
| if not stats: | ||
| log.debug("No stats for %s, skipping", sub.email) |
|
👋 This PR has had no activity for 7 days. Please push updates or comment if you still need more time. Inactive PRs may be closed automatically after 7 more days. |
Author
|
I am still working on it |
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.
Description
This PR resolves the database transaction rollback and duplicate email risk in the weekly Sunday digest background job (
_send_weekly_digests()).Currently, the job executes the dispatch loop for all active subscribers and performs a single
db.commit()at the very end. If a failure (such as an SMTP timeout, database error, or runtime exception during stats calculation) occurs mid-loop, the entire database transaction is rolled back. However, any emails already sent cannot be unsent. On the next scheduled run, those subscribers receive duplicate emails.Fixes:
db.commit()inside the loop to run immediately after each successfully sent email.try/exceptblock withdb.rollback()on failure.failedcounter and improved the run summary log.Related Issue
Fixes #1118
Type of change
Checklist
mainpytest -vand all tests passfeat/fix/docs/test: short descriptionScreenshots (if frontend change)
N/A
Test evidence
Executed the digest test suite using Python 3.12: