From 52af16c56175160512420d8654ac558a1e5af541 Mon Sep 17 00:00:00 2001
From: Erik Johnston <erikj@element.io>
Date: Sat, 18 May 2024 12:03:30 +0100
Subject: [PATCH] Add a short sleep if the request is rate-limited (#17210)

This helps prevent clients from "tight-looping" retrying their request.
---
 changelog.d/17210.misc             | 1 +
 synapse/api/ratelimiting.py        | 4 ++++
 tests/api/test_ratelimiting.py     | 5 +++--
 tests/handlers/test_federation.py  | 1 +
 tests/handlers/test_room_member.py | 4 ++++
 tests/unittest.py                  | 4 ++--
 6 files changed, 15 insertions(+), 4 deletions(-)
 create mode 100644 changelog.d/17210.misc

diff --git a/changelog.d/17210.misc b/changelog.d/17210.misc
new file mode 100644
index 0000000000..2059ebea7b
--- /dev/null
+++ b/changelog.d/17210.misc
@@ -0,0 +1 @@
+Add a short pause when rate-limiting a request.
diff --git a/synapse/api/ratelimiting.py b/synapse/api/ratelimiting.py
index a73626bc86..a99a9e09fc 100644
--- a/synapse/api/ratelimiting.py
+++ b/synapse/api/ratelimiting.py
@@ -316,6 +316,10 @@ class Ratelimiter:
         )
 
         if not allowed:
+            # We pause for a bit here to stop clients from "tight-looping" on
+            # retrying their request.
+            await self.clock.sleep(0.5)
+
             raise LimitExceededError(
                 limiter_name=self._limiter_name,
                 retry_after_ms=int(1000 * (time_allowed - time_now_s)),
diff --git a/tests/api/test_ratelimiting.py b/tests/api/test_ratelimiting.py
index a24638c9ef..a59e168db1 100644
--- a/tests/api/test_ratelimiting.py
+++ b/tests/api/test_ratelimiting.py
@@ -116,8 +116,9 @@ class TestRatelimiter(unittest.HomeserverTestCase):
         # Should raise
         with self.assertRaises(LimitExceededError) as context:
             self.get_success_or_raise(
-                limiter.ratelimit(None, key="test_id", _time_now_s=5)
+                limiter.ratelimit(None, key="test_id", _time_now_s=5), by=0.5
             )
+
         self.assertEqual(context.exception.retry_after_ms, 5000)
 
         # Shouldn't raise
@@ -192,7 +193,7 @@ class TestRatelimiter(unittest.HomeserverTestCase):
         # Second attempt, 1s later, will fail
         with self.assertRaises(LimitExceededError) as context:
             self.get_success_or_raise(
-                limiter.ratelimit(None, key=("test_id",), _time_now_s=1)
+                limiter.ratelimit(None, key=("test_id",), _time_now_s=1), by=0.5
             )
         self.assertEqual(context.exception.retry_after_ms, 9000)
 
diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py
index b819b60c5d..3fe5b0a1b4 100644
--- a/tests/handlers/test_federation.py
+++ b/tests/handlers/test_federation.py
@@ -483,6 +483,7 @@ class FederationTestCase(unittest.FederatingHomeserverTestCase):
                 event.room_version,
             ),
             exc=LimitExceededError,
+            by=0.5,
         )
 
     def _build_and_send_join_event(
diff --git a/tests/handlers/test_room_member.py b/tests/handlers/test_room_member.py
index 3e28117e2c..df43ce581c 100644
--- a/tests/handlers/test_room_member.py
+++ b/tests/handlers/test_room_member.py
@@ -70,6 +70,7 @@ class TestJoinsLimitedByPerRoomRateLimiter(FederatingHomeserverTestCase):
                 action=Membership.JOIN,
             ),
             LimitExceededError,
+            by=0.5,
         )
 
     @override_config({"rc_joins_per_room": {"per_second": 0, "burst_count": 2}})
@@ -206,6 +207,7 @@ class TestJoinsLimitedByPerRoomRateLimiter(FederatingHomeserverTestCase):
                     remote_room_hosts=[self.OTHER_SERVER_NAME],
                 ),
                 LimitExceededError,
+                by=0.5,
             )
 
     # TODO: test that remote joins to a room are rate limited.
@@ -273,6 +275,7 @@ class TestReplicatedJoinsLimitedByPerRoomRateLimiter(BaseMultiWorkerStreamTestCa
                 action=Membership.JOIN,
             ),
             LimitExceededError,
+            by=0.5,
         )
 
         # Try to join as Chris on the original worker. Should get denied because Alice
@@ -285,6 +288,7 @@ class TestReplicatedJoinsLimitedByPerRoomRateLimiter(BaseMultiWorkerStreamTestCa
                 action=Membership.JOIN,
             ),
             LimitExceededError,
+            by=0.5,
         )
 
 
diff --git a/tests/unittest.py b/tests/unittest.py
index e6aad9ed40..18963b9e32 100644
--- a/tests/unittest.py
+++ b/tests/unittest.py
@@ -637,13 +637,13 @@ class HomeserverTestCase(TestCase):
         return self.successResultOf(deferred)
 
     def get_failure(
-        self, d: Awaitable[Any], exc: Type[_ExcType]
+        self, d: Awaitable[Any], exc: Type[_ExcType], by: float = 0.0
     ) -> _TypedFailure[_ExcType]:
         """
         Run a Deferred and get a Failure from it. The failure must be of the type `exc`.
         """
         deferred: Deferred[Any] = ensureDeferred(d)  # type: ignore[arg-type]
-        self.pump()
+        self.pump(by)
         return self.failureResultOf(deferred, exc)
 
     def get_success_or_raise(self, d: Awaitable[TV], by: float = 0.0) -> TV: