From 7c83cf6fbf310e58b835bf17406ffeae8f3b4d89 Mon Sep 17 00:00:00 2001 From: edunexus-backend Date: Sat, 27 Jun 2026 20:14:59 +0530 Subject: [PATCH 1/2] fix(scheduler): isolate per-subscriber DB commit to prevent duplicate emails on failure (#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) --- backend/app/services/scheduler.py | 36 +++++--- backend/tests/test_digest.py | 134 ++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 12 deletions(-) diff --git a/backend/app/services/scheduler.py b/backend/app/services/scheduler.py index b4d80e46..5da3c75d 100644 --- a/backend/app/services/scheduler.py +++ b/backend/app/services/scheduler.py @@ -34,20 +34,32 @@ def _send_weekly_digests() -> None: return sent = 0 + failed = 0 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) + continue + ok = send_digest(stats, sub.unsubscribe_token) + if ok: + sub.last_sent_at = datetime.now(UTC) + db.commit() + sent += 1 + else: + log.warning("Failed to deliver digest to %s", sub.email) + failed += 1 + except Exception: + db.rollback() + log.exception("Error processing digest for %s — skipping", sub.email) + failed += 1 - db.commit() - log.info("Weekly digest sent to %d/%d subscribers", sent, len(subs)) + log.info( + "Weekly digest run complete: %d sent, %d failed, %d total", + sent, + failed, + len(subs), + ) except Exception: log.exception("Error in weekly digest job") finally: diff --git a/backend/tests/test_digest.py b/backend/tests/test_digest.py index 4951dfac..b45a5b3b 100644 --- a/backend/tests/test_digest.py +++ b/backend/tests/test_digest.py @@ -266,3 +266,137 @@ def test_unsubscribe_url_encodes_query_parameters(monkeypatch): assert parsed.path == "/subscribe/unsubscribe" assert query["email"] == ["digest.user+weekly@example.com"] assert query["token"] == ["token/value+with symbols"] + + +# ── Issue #1118 Regression: per-subscriber commit isolation ─────────────────── + +def _make_sub(db, email: str) -> DigestSubscription: + """Helper — insert an active DigestSubscription and return it.""" + import secrets + + sub = DigestSubscription( + email=email, + is_active=True, + unsubscribe_token=secrets.token_urlsafe(32), + ) + db.add(sub) + db.commit() + db.refresh(sub) + return sub + + +def test_digest_partial_smtp_failure_commits_successful_subscribers(monkeypatch): + """ + Regression for #1118: if one subscriber raises an SMTP error mid-loop, + the other subscribers must still have their last_sent_at committed to the DB. + Under the old single-commit design all would be rolled back, causing + duplicate emails on the next scheduled run. + """ + import smtplib + + from app.services import scheduler as scheduler_module + + db = TEST_SESSION_LOCAL() + + try: + _make_sub(db, "a@example.com") + _make_sub(db, "b@example.com") + _make_sub(db, "c@example.com") + + fake_stats = { + "email": "", + "total_analyses": 1, + "languages": ["Python"], + "avg_score": 80, + "prev_avg": 75, + "improvement": 6.7, + "trend": "up", + "top_bug": "ZeroDivisionError", + "total_issues": 1, + "week_start": "Jun 20", + "week_end": "Jun 27, 2026", + } + + def fake_compute(session, email): + return {**fake_stats, "email": email} + + call_order = [] + + def fake_send_digest(stats, token): + call_order.append(stats["email"]) + if stats["email"] == "b@example.com": + raise smtplib.SMTPException("Connection refused") + return True + + monkeypatch.setattr(scheduler_module, "SessionLocal", lambda: db) + monkeypatch.setattr(scheduler_module, "compute_subscriber_stats", fake_compute) + monkeypatch.setattr(scheduler_module, "send_digest", fake_send_digest) + monkeypatch.setattr(scheduler_module.settings, "digest_enabled", True) + + scheduler_module._send_weekly_digests() + + # Verify all 3 subscribers were attempted (order not guaranteed by SQLite) + assert set(call_order) == {"a@example.com", "b@example.com", "c@example.com"} + assert len(call_order) == 3 + + db.expire_all() + sub_a = db.query(DigestSubscription).filter_by(email="a@example.com").first() + sub_b = db.query(DigestSubscription).filter_by(email="b@example.com").first() + sub_c = db.query(DigestSubscription).filter_by(email="c@example.com").first() + + assert sub_a.last_sent_at is not None, "a@ succeeded — must be committed" + assert sub_b.last_sent_at is None, "b@ raised — must NOT be committed" + assert sub_c.last_sent_at is not None, "c@ succeeded — must be committed" + + finally: + db.close() + + +def test_digest_stats_exception_does_not_abort_other_subscribers(monkeypatch): + """ + Regression for #1118: if compute_subscriber_stats raises for one subscriber, + the remaining subscribers must still be sent and their last_sent_at committed. + """ + from app.services import scheduler as scheduler_module + + db = TEST_SESSION_LOCAL() + + try: + _make_sub(db, "fail@example.com") + _make_sub(db, "ok@example.com") + + fake_stats = { + "email": "ok@example.com", + "total_analyses": 2, + "languages": ["Python"], + "avg_score": 90, + "prev_avg": None, + "improvement": None, + "trend": "stable", + "top_bug": None, + "total_issues": 0, + "week_start": "Jun 20", + "week_end": "Jun 27, 2026", + } + + def fake_compute(session, email): + if email == "fail@example.com": + raise RuntimeError("DB exploded") + return fake_stats + + monkeypatch.setattr(scheduler_module, "SessionLocal", lambda: db) + monkeypatch.setattr(scheduler_module, "compute_subscriber_stats", fake_compute) + monkeypatch.setattr(scheduler_module, "send_digest", lambda stats, token: True) + monkeypatch.setattr(scheduler_module.settings, "digest_enabled", True) + + scheduler_module._send_weekly_digests() + + db.expire_all() + sub_fail = db.query(DigestSubscription).filter_by(email="fail@example.com").first() + sub_ok = db.query(DigestSubscription).filter_by(email="ok@example.com").first() + + assert sub_fail.last_sent_at is None, "fail@ raised — must NOT be committed" + assert sub_ok.last_sent_at is not None, "ok@ succeeded — must be committed" + + finally: + db.close() From 8faea1fed9a169f395ca53661a4da90c0039f245 Mon Sep 17 00:00:00 2001 From: edunexus-backend Date: Mon, 29 Jun 2026 15:28:17 +0530 Subject: [PATCH 2/2] fix(scheduler): resolve Copilot PR review on N+1 queries and transaction isolation --- backend/app/services/scheduler.py | 55 +++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 10 deletions(-) diff --git a/backend/app/services/scheduler.py b/backend/app/services/scheduler.py index 5da3c75d..daec1839 100644 --- a/backend/app/services/scheduler.py +++ b/backend/app/services/scheduler.py @@ -33,32 +33,67 @@ def _send_weekly_digests() -> None: log.info("No active digest subscribers") return + # Snapshot subscriber primitives to avoid N+1 reload queries and holding ORM objects active + subs_data = [ + { + "id": sub.id, + "email": sub.email, + "unsubscribe_token": sub.unsubscribe_token, + } + for sub in subs + ] + # Rollback the read transaction immediately to release locks/connection + db.rollback() + sent = 0 failed = 0 - for sub in subs: + for sub_info in subs_data: + sub_id = sub_info["id"] + email = sub_info["email"] + token = sub_info["unsubscribe_token"] + try: - stats = compute_subscriber_stats(db, sub.email) + # 1. Fetch statistics inside a transaction + stats = compute_subscriber_stats(db, email) + + # Always release the database transaction immediately after the read queries + db.rollback() + if not stats: - log.debug("No stats for %s, skipping", sub.email) + log.debug("No stats for %s, skipping", email) continue - ok = send_digest(stats, sub.unsubscribe_token) + + # 2. Trigger the SMTP network call outside any open database transaction + ok = send_digest(stats, token) + if ok: - sub.last_sent_at = datetime.now(UTC) - db.commit() - sent += 1 + # 3. Update the specific subscriber's timestamp in a fresh, isolated transaction + sub_record = ( + db.query(DigestSubscription) + .filter(DigestSubscription.id == sub_id) + .first() + ) + if sub_record: + sub_record.last_sent_at = datetime.now(UTC) + db.commit() + sent += 1 + else: + db.rollback() + log.warning("Subscriber record ID %d not found during update", sub_id) + failed += 1 else: - log.warning("Failed to deliver digest to %s", sub.email) + log.warning("Failed to deliver digest to %s", email) failed += 1 except Exception: db.rollback() - log.exception("Error processing digest for %s — skipping", sub.email) + log.exception("Error processing digest for %s — skipping", email) failed += 1 log.info( "Weekly digest run complete: %d sent, %d failed, %d total", sent, failed, - len(subs), + len(subs_data), ) except Exception: log.exception("Error in weekly digest job")