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
This commit is contained in:
Alex Vandiver 2021-08-17 01:21:10 +00:00
parent 717c26d82c
commit 9d97af6ebb

View File

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