Repository: zulip/zulip
Pull request: push_notifs: Make push_notifications_enabled more resistant to flapping.
Fixes #28403

Uses redis to remember the last time push notifications were experienced working. This needs to work across processes, so can't be done just in memory.



Problem statement:
Make `push_notifications_enabled` more resistant to flapping
Edited to remove the part that we determined is not actually a bug.

~~The `maybe_mark_pushes_disabled` logic is asymmetric, in that it checks if push notifications appear to not be working on every request, but only consider marking them as disabled -- they will only be switched to enabled when the hourly cron job runs.~~

~~This is not intended and needs to be fixed for 8.1.~~

It might also be nice to use some local state to avoid flapping with networking issues; if there have been both successful and unsuccessful requests to the push bouncer service in the last hour, then a (potentially transient) networking failure trying to send a push notification should probably not mark push notifications as not working.

Hints from issue discussion:
Is the conclusion that this issue was opened in error because we already have that logic?
@timabbott  I believe so. The other point raised here:

> It might also be nice to use some local state to avoid flapping with networking issues; if there have been both successful and unsuccessful requests to the push bouncer service in the last hour, then a (potentially transient) networking failure trying to send a push notification should probably not mark push notifications as not working.

May warrant keeping this issue up after renaming it, or just opening a new one. Not sure if the plan would still be to try to merge this for 8.1 if I can make a PR for it in time?
I think we can leave that for 8.2. I'll retitle the issue.
@mateuszmandera You have been unassigned from this issue because you have not made any updates for over 14 days. Please feel free to reclaim the issue if you decide to pick up again. Thanks!

@timabbott, @mateuszmandera You have been unassigned from this issue because you have not made any updates for over 14 days. Please feel free to reclaim the issue if you decide to pick up again. Thanks!

@mateuszmandera You have been unassigned from this issue because you have not made any updates for over 14 days. Please feel free to reclaim the issue if you decide to pick up again. Thanks!

Commit message: rate_limiter: Extract KEY_PREFIX to redis_utils.

Patch to review:
diff --git a/zerver/lib/rate_limiter.py b/zerver/lib/rate_limiter.py
index c086b1fe3e0a3..6381058a72385 100644
--- a/zerver/lib/rate_limiter.py
+++ b/zerver/lib/rate_limiter.py
@@ -1,5 +1,4 @@
 import logging
-import os
 import time
 from abc import ABC, abstractmethod
 from typing import Dict, List, Optional, Set, Tuple, Type, cast
@@ -11,6 +10,7 @@
 from django.http import HttpRequest
 from typing_extensions import override
 
+from zerver.lib import redis_utils
 from zerver.lib.cache import cache_with_key
 from zerver.lib.exceptions import RateLimitedError
 from zerver.lib.redis_utils import get_redis_client
@@ -22,8 +22,6 @@
 client = get_redis_client()
 rules: Dict[str, List[Tuple[int, int]]] = settings.RATE_LIMITING_RULES
 
-KEY_PREFIX = ""
-
 logger = logging.getLogger(__name__)
 
 
@@ -159,11 +157,6 @@ def rules(self) -> List[Tuple[int, int]]:
         return rules[self.domain]
 
 
-def bounce_redis_key_prefix_for_testing(test_name: str) -> None:
-    global KEY_PREFIX
-    KEY_PREFIX = test_name + ":" + str(os.getpid()) + ":"
-
-
 class RateLimiterBackend(ABC):
     @classmethod
     @abstractmethod
@@ -323,7 +316,8 @@ class RedisRateLimiterBackend(RateLimiterBackend):
     @classmethod
     def get_keys(cls, entity_key: str) -> List[str]:
         return [
-            f"{KEY_PREFIX}ratelimit:{entity_key}:{keytype}" for keytype in ["list", "zset", "block"]
+            f"{redis_utils.REDIS_KEY_PREFIX}ratelimit:{entity_key}:{keytype}"
+            for keytype in ["list", "zset", "block"]
         ]
 
     @classmethod
diff --git a/zerver/lib/redis_utils.py b/zerver/lib/redis_utils.py
index 16d9100bb07f9..deddca2a091c1 100644
--- a/zerver/lib/redis_utils.py
+++ b/zerver/lib/redis_utils.py
@@ -1,3 +1,4 @@
+import os
 import re
 import secrets
 from typing import Any, Dict, Mapping, Optional
@@ -84,3 +85,11 @@ def validate_key_fits_format(key: str, key_format: str) -> None:
 
     if not re.fullmatch(regex, key):
         raise ZulipRedisKeyOfWrongFormatError(f"{key} does not match format {key_format}")
+
+
+REDIS_KEY_PREFIX = ""
+
+
+def bounce_redis_key_prefix_for_testing(test_name: str) -> None:
+    global REDIS_KEY_PREFIX
+    REDIS_KEY_PREFIX = test_name + ":" + str(os.getpid()) + ":"
diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py
index d897f5a6dbb06..c8be32bef4801 100644
--- a/zerver/lib/test_classes.py
+++ b/zerver/lib/test_classes.py
@@ -66,7 +66,7 @@
 from zerver.lib.message import access_message
 from zerver.lib.notification_data import UserMessageNotificationsData
 from zerver.lib.per_request_cache import flush_per_request_caches
-from zerver.lib.rate_limiter import bounce_redis_key_prefix_for_testing
+from zerver.lib.redis_utils import bounce_redis_key_prefix_for_testing
 from zerver.lib.sessions import get_session_dict_user
 from zerver.lib.soft_deactivation import do_soft_deactivate_users
 from zerver.lib.stream_subscription import get_subscribed_stream_ids_for_user


Code review comments:
diff_hunk: @@ -245,6 +250,29 @@ def send_json_to_push_bouncer(
     )
 
 
+PUSH_NOTIFICATIONS_RECENTLY_WORKING_REDIS_KEY = "push_notifications_recently_working_ts"
+
+
+def record_push_notifications_recently_working() -> None:
+    # Record the timestamp in redisp, marking that push notifications
path: zerver/lib/remote_server.py
text: @user1:
```suggestion
    # Record the timestamp in redis, marking that push notifications
```