push notif: Don't forget to clear "active" flag on sending to bouncer.

Since da8f4bc0e back in August, this control flow has caused
`flags.active_mobile_push_notification` to be cleared if we don't send
these `remove` messages at all, and if we send them directly to GCM...
but not if we send them via the Zulip notification bouncer.

As a result, on a server configured to send `remove` notification-messages
via the bouncer, we accumulate "active" messages and never clear them.

If the user then does `mark_all_as_read`, we end up sending a `remove`
for each of those messages again, and all in one giant burst.  We've
seen puzzling bursts of hundreds of removals pass through the bouncer
since turning on removals on chat.zulip.org; it's likely many of them
are caused by this bug.

This issue was made more acute with f4478aad5, which unconditionally
enabled removals.

Test added by tabbott.

zerver/lib/push_notifications.py

@@ -620,13 +620,11 @@ def handle_remove_push_notification(user_profile_id: int, message_id: int) -> No
                 logger.warning(
                     "Maximum retries exceeded for trigger:%s event:push_notification" % (
                         event['user_profile_id']))
-        return
-
-    android_devices = list(PushDeviceToken.objects.filter(user=user_profile,
-                                                          kind=PushDeviceToken.GCM))
-
-    if android_devices:
-        send_android_push_notification(android_devices, gcm_payload, gcm_options)
+    else:
+        android_devices = list(PushDeviceToken.objects.filter(
+            user=user_profile, kind=PushDeviceToken.GCM))
+        if android_devices:
+            send_android_push_notification(android_devices, gcm_payload, gcm_options)
 
     user_message.flags.active_mobile_push_notification = False
     user_message.save(update_fields=["flags"])

zerver/tests/test_push_notifications.py

@@ -717,7 +717,8 @@ class HandlePushNotificationTest(PushNotificationTest):
         message = self.get_message(Recipient.PERSONAL, type_id=1)
         UserMessage.objects.create(
             user_profile=user_profile,
-            message=message
+            message=message,
+            flags=UserMessage.flags.active_mobile_push_notification,
         )
 
         with self.settings(PUSH_NOTIFICATION_BOUNCER_URL=True), \
@@ -731,6 +732,9 @@ class HandlePushNotificationTest(PushNotificationTest):
                                                   'event': 'remove',
                                                   'zulip_message_id': message.id},
                                                  {'priority': 'normal'})
+            user_message = UserMessage.objects.get(user_profile=self.user_profile,
+                                                   message=message)
+            self.assertEqual(user_message.flags.active_mobile_push_notification, False)
 
     def test_non_bouncer_push_remove(self) -> None:
         self.setup_apns_tokens()
@@ -738,7 +742,8 @@ class HandlePushNotificationTest(PushNotificationTest):
         message = self.get_message(Recipient.PERSONAL, type_id=1)
         UserMessage.objects.create(
             user_profile=self.user_profile,
-            message=message
+            message=message,
+            flags=UserMessage.flags.active_mobile_push_notification,
         )
 
         android_devices = list(
@@ -755,6 +760,9 @@ class HandlePushNotificationTest(PushNotificationTest):
                                                   'event': 'remove',
                                                   'zulip_message_id': message.id},
                                                  {'priority': 'normal'})
+            user_message = UserMessage.objects.get(user_profile=self.user_profile,
+                                                   message=message)
+            self.assertEqual(user_message.flags.active_mobile_push_notification, False)
 
     def test_user_message_does_not_exist(self) -> None:
         """This simulates a condition that should only be an error if the user is