Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

push_notifications: Lock message while we mark it pending for push. 

Deleting a message can race with sending a push notification for it.
b47535d8bb handled the case where the Message row has gone away --
but in such cases, it is also possible for `access_message` to
succeed, but for the save of `user_message.flags` to fail, because the
UserMessage row has been deleted by then.

Take a lock on the Message row over the accesses of, and updates to,
the relevant UserMessage row.  This guarantees that the
message's (non-)existence is consistent across that transaction.

Partial fix for #16502.
This commit is contained in:
Alex Vandiver 2023-05-18 15:51:21 +00:00 committed by Tim Abbott
parent 12310189ed
commit 1184bdc934
1 changed files with 36 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
zerver/lib/push_notifications.py
View File

@ -1060,43 +1060,46 @@ def handle_push_notification(user_profile_id: int, missed_message: Dict[str, Any
# BUG: Investigate why it's possible to get here.
return # nocoverage
try:
(message, user_message) = access_message(user_profile, missed_message["message_id"])
except JsonableError:
if ArchivedMessage.objects.filter(id=missed_message["message_id"]).exists():
# If the cause is a race with the message being deleted,
# that's normal and we have no need to log an error.
return
logging.info(
"Unexpected message access failure handling push notifications: %s %s",
user_profile.id,
missed_message["message_id"],
)
return
if user_message is not None:
# If the user has read the message already, don't push-notify.
if user_message.flags.read or user_message.flags.active_mobile_push_notification:
return
# Otherwise, we mark the message as having an active mobile
# push notification, so that we can send revocation messages
# later.
user_message.flags.active_mobile_push_notification = True
user_message.save(update_fields=["flags"])
else:
# Users should only be getting push notifications into this
# queue for messages they haven't received if they're
# long-term idle; anything else is likely a bug.
if not user_profile.long_term_idle:
logger.error(
"Could not find UserMessage with message_id %s and user_id %s",
with transaction.atomic(savepoint=False):
try:
(message, user_message) = access_message(
user_profile, missed_message["message_id"], lock_message=True
)
except JsonableError:
if ArchivedMessage.objects.filter(id=missed_message["message_id"]).exists():
# If the cause is a race with the message being deleted,
# that's normal and we have no need to log an error.
return
logging.info(
"Unexpected message access failure handling push notifications: %s %s",
user_profile.id,
missed_message["message_id"],
user_profile_id,
exc_info=True,
)
return
if user_message is not None:
# If the user has read the message already, don't push-notify.
if user_message.flags.read or user_message.flags.active_mobile_push_notification:
return
# Otherwise, we mark the message as having an active mobile
# push notification, so that we can send revocation messages
# later.
user_message.flags.active_mobile_push_notification = True
user_message.save(update_fields=["flags"])
else:
# Users should only be getting push notifications into this
# queue for messages they haven't received if they're
# long-term idle; anything else is likely a bug.
if not user_profile.long_term_idle:
logger.error(
"Could not find UserMessage with message_id %s and user_id %s",
missed_message["message_id"],
user_profile_id,
exc_info=True,
)
return
trigger = missed_message["trigger"]
mentioned_user_group_name = None
# mentioned_user_group_id will be None if the user is personally mentioned