From 9d97af6ebba9f6906cb0e477aea33edc44aa4770 Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@zulip.com>
Date: Tue, 17 Aug 2021 01:21:10 +0000
Subject: [PATCH] scheduled_email: Create ScheduledEmail objects in a
 transaction.

This fixes two bugs: the most obvious is that there is a race where a
ScheduledEmail object could be observed in the window between creation
and when users are added; this is a momentary instance when the object
has no users, but one that will resolve itself.

The more subtle is that .save() will, if no records were found to be
updated, _re-create_ the object as it exists in memory, using an
INSERT[1].  Thus, there is a race with `deliver_scheduled_emails`
between when the users are added, and when `email.save()` runs:

 1. Web request creates ScheduledEmail object
 2. Web request creates ScheduledEmailUsers object
 3. deliver_scheduled_emails locks the former, preventing updates.
 4. deliver_scheduled_emails deletes both objects, commits, releasing lock
 5. Web request calls `email.save()`; UPDATE finds no rows, so it
    re-creates the ScheduledEmail object.
 6. Future deliver_scheduled_emails runs find a ScheduledEmail with no
    attending ScheduledEmailUsers objects

Wrapping the logical creation of both of these in a single transaction
avoids both of these races.

[1] https://docs.djangoproject.com/en/3.2/ref/models/instances/#how-django-knows-to-update-vs-insert
---
 zerver/lib/send_email.py | 41 ++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/zerver/lib/send_email.py b/zerver/lib/send_email.py
index 344034d522..385625d070 100644
--- a/zerver/lib/send_email.py
+++ b/zerver/lib/send_email.py
@@ -345,27 +345,28 @@ def send_future_email(
         # For logging the email
 
     assert (to_user_ids is None) ^ (to_emails is None)
-    email = ScheduledEmail.objects.create(
-        type=EMAIL_TYPES[template_name],
-        scheduled_timestamp=timezone_now() + delay,
-        realm=realm,
-        data=orjson.dumps(email_fields).decode(),
-    )
+    with transaction.atomic():
+        email = ScheduledEmail.objects.create(
+            type=EMAIL_TYPES[template_name],
+            scheduled_timestamp=timezone_now() + delay,
+            realm=realm,
+            data=orjson.dumps(email_fields).decode(),
+        )
 
-    # We store the recipients in the ScheduledEmail object itself,
-    # rather than the JSON data object, so that we can find and clear
-    # them using clear_scheduled_emails.
-    try:
-        if to_user_ids is not None:
-            email.users.add(*to_user_ids)
-        else:
-            assert to_emails is not None
-            assert len(to_emails) == 1
-            email.address = parseaddr(to_emails[0])[1]
-        email.save()
-    except Exception as e:
-        email.delete()
-        raise e
+        # We store the recipients in the ScheduledEmail object itself,
+        # rather than the JSON data object, so that we can find and clear
+        # them using clear_scheduled_emails.
+        try:
+            if to_user_ids is not None:
+                email.users.add(*to_user_ids)
+            else:
+                assert to_emails is not None
+                assert len(to_emails) == 1
+                email.address = parseaddr(to_emails[0])[1]
+            email.save()
+        except Exception as e:
+            email.delete()
+            raise e
 
 
 def send_email_to_admins(