fix(scheduler): isolate per-subscriber DB commit to prevent duplicate emails (#1118)

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:

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()