From 042841ee3a5524d8aab1ca2b29734e66d85d6bc1 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Wed, 28 Sep 2022 17:30:54 +0100
Subject: [PATCH 01/13] Reproduce bug

Test with
```
SYNAPSE_TEST_LOG_LEVEL=DEBUG SYNAPSE_POSTGRES=1 SYNAPSE_POSTGRES_HOST=127.0.0.1 SYNAPSE_POSTGRES_USER=postgres SYNAPSE_POSTGRES_PASSWORD=postgres poetry run trial tests.storage.test_event_federation.EventFederationWorkerStoreTestCase.test_repeated_backfill_failures_are_retried_appropriately
```
having setup a postgres server with
```
docker run --rm -e POSTGRES_PASSWORD=postgres -e POSTGRES_USER=postgres -e POSTGRES_DB=synapse -p 5432:5432 postgres:10-alpine
```
---
 tests/storage/test_event_federation.py | 40 ++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tests/storage/test_event_federation.py b/tests/storage/test_event_federation.py
index 85739c464ec7..5d650132757a 100644
--- a/tests/storage/test_event_federation.py
+++ b/tests/storage/test_event_federation.py
@@ -1049,6 +1049,46 @@ def test_get_insertion_event_backward_extremities_in_room_attempted_event_retry_
             backfill_event_ids, ["insertion_eventB", "insertion_eventA"]
         )
 
+    def test_repeated_backfill_failures_are_retried_appropriately(self) -> None:
+        """
+        A test that reproduces #13929 (Postgres only).
+
+        We should be able to backoff repeatedly, even if the backoff formula would tell
+        us to wait for more seconds than can be expressed in a 32 bit signed int.
+        """
+        setup_info = self._setup_room_for_insertion_backfill_tests()
+        room_id = setup_info.room_id
+
+        # Pretend that we have tried and failed 10 times to backfill event A.
+        for _ in range(10):
+            self.get_success(
+                self.store.record_event_failed_pull_attempt(
+                    room_id, "insertion_eventA", "fake cause"
+                )
+            )
+
+        # If the backoff periods grow without limit:
+        # After the first failed attempt, we would have backed off for 1 << 1 = 2 hours.
+        # After the second failed attempt we would have backed off for 1 << 2 = 4 hours,
+        # so after the 10th failed attempt we should backoff for 1 << 10 == 1024 hours.
+        # Wait 1100 hours just so we have a nice round number.
+        self.reactor.advance(datetime.timedelta(hours=1100).total_seconds())
+
+        # 1024 hours in milliseconds is 1024 * 3600000, which exceeds the largest 32 bit
+        # signed integer. The bug we're reproducing is that this overflow causes an
+        # error in postgres preventing us from fetching a set of backwards extremities
+        # to retry fetching.
+        backfill_points = self.get_success(
+            self.store.get_insertion_event_backward_extremities_in_room(room_id)
+        )
+
+        # We should aim to fetch A (the backoff period has expired) and to fetch B
+        # (we haven't tried to fetch it yet).
+        backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
+        self.assertListEqual(
+            backfill_event_ids, ["insertion_eventB", "insertion_eventA"]
+        )
+
 
 @attr.s
 class FakeEvent:

From 9412bf684b99b2e9cae04543e60a69de8fe86e7b Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Wed, 28 Sep 2022 17:33:07 +0100
Subject: [PATCH 02/13] Compute `least_function` first

---
 .../databases/main/event_federation.py        | 29 ++++++++++---------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py
index 3251fca6fbf2..6df66e4e0acb 100644
--- a/synapse/storage/databases/main/event_federation.py
+++ b/synapse/storage/databases/main/event_federation.py
@@ -749,6 +749,14 @@ def get_backfill_points_in_room_txn(
             # persisted in our database yet (meaning we don't know their depth
             # specifically). So we need to look for the approximate depth from
             # the events connected to the current backwards extremeties.
+
+            if isinstance(self.database_engine, PostgresEngine):
+                least_function = "least"
+            elif isinstance(self.database_engine, Sqlite3Engine):
+                least_function = "min"
+            else:
+                raise RuntimeError("Unknown database engine")
+
             sql = """
                 SELECT backward_extrem.event_id, event.depth FROM events AS event
                 /**
@@ -805,13 +813,6 @@ def get_backfill_points_in_room_txn(
                 ORDER BY event.depth DESC, backward_extrem.event_id DESC
             """
 
-            if isinstance(self.database_engine, PostgresEngine):
-                least_function = "least"
-            elif isinstance(self.database_engine, Sqlite3Engine):
-                least_function = "min"
-            else:
-                raise RuntimeError("Unknown database engine")
-
             txn.execute(
                 sql % (least_function,),
                 (
@@ -852,6 +853,13 @@ async def get_insertion_event_backward_extremities_in_room(
         def get_insertion_event_backward_extremities_in_room_txn(
             txn: LoggingTransaction, room_id: str
         ) -> List[Tuple[str, int]]:
+            if isinstance(self.database_engine, PostgresEngine):
+                least_function = "least"
+            elif isinstance(self.database_engine, Sqlite3Engine):
+                least_function = "min"
+            else:
+                raise RuntimeError("Unknown database engine")
+
             sql = """
                 SELECT
                     insertion_event_extremity.event_id, event.depth
@@ -890,13 +898,6 @@ def get_insertion_event_backward_extremities_in_room_txn(
                 ORDER BY event.depth DESC, insertion_event_extremity.event_id DESC
             """
 
-            if isinstance(self.database_engine, PostgresEngine):
-                least_function = "least"
-            elif isinstance(self.database_engine, Sqlite3Engine):
-                least_function = "min"
-            else:
-                raise RuntimeError("Unknown database engine")
-
             txn.execute(
                 sql % (least_function,),
                 (

From 83f4e52e212cd36032fe5c91783ed1ed3bb007c5 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Wed, 28 Sep 2022 17:33:47 +0100
Subject: [PATCH 03/13] Substitute `least_function` with an f-string

---
 synapse/storage/databases/main/event_federation.py | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py
index 6df66e4e0acb..f2ad6800691c 100644
--- a/synapse/storage/databases/main/event_federation.py
+++ b/synapse/storage/databases/main/event_federation.py
@@ -757,7 +757,7 @@ def get_backfill_points_in_room_txn(
             else:
                 raise RuntimeError("Unknown database engine")
 
-            sql = """
+            sql = f"""
                 SELECT backward_extrem.event_id, event.depth FROM events AS event
                 /**
                  * Get the edge connections from the event_edges table
@@ -803,7 +803,7 @@ def get_backfill_points_in_room_txn(
                      */
                     AND (
                         failed_backfill_attempt_info.event_id IS NULL
-                        OR ? /* current_time */ >= failed_backfill_attempt_info.last_attempt_ts + /*least*/%s((1 << failed_backfill_attempt_info.num_attempts) * ? /* step */, ? /* upper bound */)
+                        OR ? /* current_time */ >= failed_backfill_attempt_info.last_attempt_ts + {least_function}((1 << failed_backfill_attempt_info.num_attempts) * ? /* step */, ? /* upper bound */)
                     )
                 /**
                  * Sort from highest to the lowest depth. Then tie-break on
@@ -814,7 +814,7 @@ def get_backfill_points_in_room_txn(
             """
 
             txn.execute(
-                sql % (least_function,),
+                sql,
                 (
                     room_id,
                     False,
@@ -860,7 +860,7 @@ def get_insertion_event_backward_extremities_in_room_txn(
             else:
                 raise RuntimeError("Unknown database engine")
 
-            sql = """
+            sql = f"""
                 SELECT
                     insertion_event_extremity.event_id, event.depth
                 /* We only want insertion events that are also marked as backwards extremities */
@@ -888,7 +888,7 @@ def get_insertion_event_backward_extremities_in_room_txn(
                      */
                     AND (
                         failed_backfill_attempt_info.event_id IS NULL
-                        OR ? /* current_time */ >= failed_backfill_attempt_info.last_attempt_ts + /*least*/%s((1 << failed_backfill_attempt_info.num_attempts) * ? /* step */, ? /* upper bound */)
+                        OR ? /* current_time */ >= failed_backfill_attempt_info.last_attempt_ts + {least_function}((1 << failed_backfill_attempt_info.num_attempts) * ? /* step */, ? /* upper bound */)
                     )
                 /**
                  * Sort from highest to the lowest depth. Then tie-break on
@@ -899,7 +899,7 @@ def get_insertion_event_backward_extremities_in_room_txn(
             """
 
             txn.execute(
-                sql % (least_function,),
+                sql,
                 (
                     room_id,
                     self._clock.time_msec(),

From 4a0c4612c712fc553d587b3e8ac0a12f3017defc Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Wed, 28 Sep 2022 17:34:34 +0100
Subject: [PATCH 04/13] Bugfix: avoid overflow

---
 .../databases/main/event_federation.py        | 28 ++++++++++++++-----
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py
index f2ad6800691c..6b99eb59d853 100644
--- a/synapse/storage/databases/main/event_federation.py
+++ b/synapse/storage/databases/main/event_federation.py
@@ -73,12 +73,20 @@
 
 logger = logging.getLogger(__name__)
 
-BACKFILL_EVENT_BACKOFF_UPPER_BOUND_SECONDS: int = int(
-    datetime.timedelta(days=7).total_seconds()
-)
+BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS = 8
 BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_SECONDS: int = int(
     datetime.timedelta(hours=1).total_seconds()
 )
+# The longest backoff period is then:
+# _LONGEST_BACKOFF_PERIOD_SECONDS = (
+#     (1 << BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS)
+#     * BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_SECONDS,
+# )
+# which is 2 ** 8 hours = 256 hours ≈ 10 days. This number needs to fit
+# in a 32 bit signed int, or else Postgres will error.
+# assert _LONGEST_BACKOFF_PERIOD_SECONDS < ((2 ** 31) - 1)
+# (We could use a bigint, but bigint overflows still cause errors. This
+# at least avoids CASTing in the queries below.)
 
 
 # All the info we need while iterating the DAG while backfilling
@@ -803,7 +811,10 @@ def get_backfill_points_in_room_txn(
                      */
                     AND (
                         failed_backfill_attempt_info.event_id IS NULL
-                        OR ? /* current_time */ >= failed_backfill_attempt_info.last_attempt_ts + {least_function}((1 << failed_backfill_attempt_info.num_attempts) * ? /* step */, ? /* upper bound */)
+                        OR ? /* current_time */ >= failed_backfill_attempt_info.last_attempt_ts + (
+                            (1 << {least_function}(failed_backfill_attempt_info.num_attempts, ? /* max doubling steps */))
+                            * ? /* step */
+                        )
                     )
                 /**
                  * Sort from highest to the lowest depth. Then tie-break on
@@ -819,8 +830,8 @@ def get_backfill_points_in_room_txn(
                     room_id,
                     False,
                     self._clock.time_msec(),
+                    BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS,
                     1000 * BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_SECONDS,
-                    1000 * BACKFILL_EVENT_BACKOFF_UPPER_BOUND_SECONDS,
                 ),
             )
 
@@ -888,7 +899,10 @@ def get_insertion_event_backward_extremities_in_room_txn(
                      */
                     AND (
                         failed_backfill_attempt_info.event_id IS NULL
-                        OR ? /* current_time */ >= failed_backfill_attempt_info.last_attempt_ts + {least_function}((1 << failed_backfill_attempt_info.num_attempts) * ? /* step */, ? /* upper bound */)
+                        OR ? /* current_time */ >= failed_backfill_attempt_info.last_attempt_ts + (
+                            (1 << {least_function}(failed_backfill_attempt_info.num_attempts, ? /* max doubling steps */))
+                            * ? /* step */
+                        )
                     )
                 /**
                  * Sort from highest to the lowest depth. Then tie-break on
@@ -903,8 +917,8 @@ def get_insertion_event_backward_extremities_in_room_txn(
                 (
                     room_id,
                     self._clock.time_msec(),
+                    BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS,
                     1000 * BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_SECONDS,
-                    1000 * BACKFILL_EVENT_BACKOFF_UPPER_BOUND_SECONDS,
                 ),
             )
             return cast(List[Tuple[str, int]], txn.fetchall())

From 2a800fc85a4c9d1ed0042604d738ed65ddea1446 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Wed, 28 Sep 2022 18:09:18 +0100
Subject: [PATCH 05/13] Changelog

---
 changelog.d/13936.feature | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/13936.feature

diff --git a/changelog.d/13936.feature b/changelog.d/13936.feature
new file mode 100644
index 000000000000..d86bf7ed809f
--- /dev/null
+++ b/changelog.d/13936.feature
@@ -0,0 +1 @@
+Exponentially backoff from backfilling the same event over and over.

From 4f6637dbd76a3197a0ad89476a6a13ecc33bda4a Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 29 Sep 2022 20:12:29 +0100
Subject: [PATCH 06/13] Drive-by: prefer `assertEqual` over `assertListEqual`

---
 tests/storage/test_event_federation.py | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/storage/test_event_federation.py b/tests/storage/test_event_federation.py
index 5d650132757a..bfedb3b7317a 100644
--- a/tests/storage/test_event_federation.py
+++ b/tests/storage/test_event_federation.py
@@ -763,7 +763,7 @@ def test_get_backfill_points_in_room(self):
             self.store.get_backfill_points_in_room(room_id)
         )
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
-        self.assertListEqual(
+        self.assertEqual(
             backfill_event_ids, ["b6", "b5", "b4", "2", "b3", "b2", "b1"]
         )
 
@@ -800,7 +800,7 @@ def test_get_backfill_points_in_room_excludes_events_we_have_attempted(
         )
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
         # Only the backfill points that we didn't record earlier exist here.
-        self.assertListEqual(backfill_event_ids, ["b6", "2", "b1"])
+        self.assertEqual(backfill_event_ids, ["b6", "2", "b1"])
 
     def test_get_backfill_points_in_room_attempted_event_retry_after_backoff_duration(
         self,
@@ -845,7 +845,7 @@ def test_get_backfill_points_in_room_attempted_event_retry_after_backoff_duratio
             self.store.get_backfill_points_in_room(room_id)
         )
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
-        self.assertListEqual(backfill_event_ids, ["b6", "b5", "b4", "2", "b3", "b2"])
+        self.assertEqual(backfill_event_ids, ["b6", "b5", "b4", "2", "b3", "b2"])
 
         # Now advance time by 20 hours (above 2^4 because we made 4 attemps) and
         # see if we can now backfill it
@@ -856,7 +856,7 @@ def test_get_backfill_points_in_room_attempted_event_retry_after_backoff_duratio
             self.store.get_backfill_points_in_room(room_id)
         )
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
-        self.assertListEqual(
+        self.assertEqual(
             backfill_event_ids, ["b6", "b5", "b4", "2", "b3", "b2", "b1"]
         )
 
@@ -947,7 +947,7 @@ def test_get_insertion_event_backward_extremities_in_room(self):
             self.store.get_insertion_event_backward_extremities_in_room(room_id)
         )
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
-        self.assertListEqual(
+        self.assertEqual(
             backfill_event_ids, ["insertion_eventB", "insertion_eventA"]
         )
 
@@ -978,7 +978,7 @@ def test_get_insertion_event_backward_extremities_in_room_excludes_events_we_hav
         )
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
         # Only the backfill points that we didn't record earlier exist here.
-        self.assertListEqual(backfill_event_ids, ["insertion_eventB"])
+        self.assertEqual(backfill_event_ids, ["insertion_eventB"])
 
     def test_get_insertion_event_backward_extremities_in_room_attempted_event_retry_after_backoff_duration(
         self,
@@ -1033,7 +1033,7 @@ def test_get_insertion_event_backward_extremities_in_room_attempted_event_retry_
             self.store.get_insertion_event_backward_extremities_in_room(room_id)
         )
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
-        self.assertListEqual(backfill_event_ids, ["insertion_eventB"])
+        self.assertEqual(backfill_event_ids, ["insertion_eventB"])
 
         # Now advance time by 20 hours (above 2^4 because we made 4 attemps) and
         # see if we can now backfill it
@@ -1045,7 +1045,7 @@ def test_get_insertion_event_backward_extremities_in_room_attempted_event_retry_
             self.store.get_insertion_event_backward_extremities_in_room(room_id)
         )
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
-        self.assertListEqual(
+        self.assertEqual(
             backfill_event_ids, ["insertion_eventB", "insertion_eventA"]
         )
 
@@ -1085,7 +1085,7 @@ def test_repeated_backfill_failures_are_retried_appropriately(self) -> None:
         # We should aim to fetch A (the backoff period has expired) and to fetch B
         # (we haven't tried to fetch it yet).
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
-        self.assertListEqual(
+        self.assertEqual(
             backfill_event_ids, ["insertion_eventB", "insertion_eventA"]
         )
 

From c392efc278944e082c137efa8efb0bde2502af89 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 29 Sep 2022 20:13:40 +0100
Subject: [PATCH 07/13] prefer `LEAST` over `least`

---
 synapse/storage/databases/main/event_federation.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py
index 6b99eb59d853..62a1d728d9f2 100644
--- a/synapse/storage/databases/main/event_federation.py
+++ b/synapse/storage/databases/main/event_federation.py
@@ -759,9 +759,9 @@ def get_backfill_points_in_room_txn(
             # the events connected to the current backwards extremeties.
 
             if isinstance(self.database_engine, PostgresEngine):
-                least_function = "least"
+                least_function = "LEAST"
             elif isinstance(self.database_engine, Sqlite3Engine):
-                least_function = "min"
+                least_function = "MIN"
             else:
                 raise RuntimeError("Unknown database engine")
 
@@ -865,9 +865,9 @@ def get_insertion_event_backward_extremities_in_room_txn(
             txn: LoggingTransaction, room_id: str
         ) -> List[Tuple[str, int]]:
             if isinstance(self.database_engine, PostgresEngine):
-                least_function = "least"
+                least_function = "LEAST"
             elif isinstance(self.database_engine, Sqlite3Engine):
-                least_function = "min"
+                least_function = "MIN"
             else:
                 raise RuntimeError("Unknown database engine")
 

From c713c8ec784e436ffe6853959950bbcebefc9be9 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 29 Sep 2022 20:20:32 +0100
Subject: [PATCH 08/13] Make the constant hold milliseconds

---
 synapse/storage/databases/main/event_federation.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py
index 62a1d728d9f2..dbebfad42043 100644
--- a/synapse/storage/databases/main/event_federation.py
+++ b/synapse/storage/databases/main/event_federation.py
@@ -74,8 +74,8 @@
 logger = logging.getLogger(__name__)
 
 BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS = 8
-BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_SECONDS: int = int(
-    datetime.timedelta(hours=1).total_seconds()
+BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_MILLISECONDS = int(
+    datetime.timedelta(hours=1).total_seconds() * 1000
 )
 # The longest backoff period is then:
 # _LONGEST_BACKOFF_PERIOD_SECONDS = (
@@ -831,7 +831,7 @@ def get_backfill_points_in_room_txn(
                     False,
                     self._clock.time_msec(),
                     BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS,
-                    1000 * BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_SECONDS,
+                    BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_MILLISECONDS,
                 ),
             )
 
@@ -918,7 +918,7 @@ def get_insertion_event_backward_extremities_in_room_txn(
                     room_id,
                     self._clock.time_msec(),
                     BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS,
-                    1000 * BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_SECONDS,
+                    BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_MILLISECONDS,
                 ),
             )
             return cast(List[Tuple[str, int]], txn.fetchall())

From a91d8101d04a1794dd589c6e5179721e488068a2 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 29 Sep 2022 20:38:10 +0100
Subject: [PATCH 09/13] Update explanatory comment; add assertion

---
 .../databases/main/event_federation.py        | 29 ++++++++++++-------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py
index dbebfad42043..ffa92c650bb0 100644
--- a/synapse/storage/databases/main/event_federation.py
+++ b/synapse/storage/databases/main/event_federation.py
@@ -73,20 +73,29 @@
 
 logger = logging.getLogger(__name__)
 
+# Parameters controlling exponential backoff between backfill failures.
+# After the first failure to backfill, we wait 2 hours before trying again. If the
+# second attempt fails, we wait 4 hours before trying again. If the third attempt fails,
+# we wait 8 hours before trying again, ... and so on.
+#
+# Each successive backoff period is twice as long as the last. However we cap this
+# period at a maximum of 2^8 = 256 hours: a little over 10 days. (This is the smallest
+# power of 2 which yields a maximum backoff period of at least 7 days---which was the
+# original maximum backoff period.)
 BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS = 8
 BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_MILLISECONDS = int(
     datetime.timedelta(hours=1).total_seconds() * 1000
 )
-# The longest backoff period is then:
-# _LONGEST_BACKOFF_PERIOD_SECONDS = (
-#     (1 << BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS)
-#     * BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_SECONDS,
-# )
-# which is 2 ** 8 hours = 256 hours ≈ 10 days. This number needs to fit
-# in a 32 bit signed int, or else Postgres will error.
-# assert _LONGEST_BACKOFF_PERIOD_SECONDS < ((2 ** 31) - 1)
-# (We could use a bigint, but bigint overflows still cause errors. This
-# at least avoids CASTing in the queries below.)
+
+# We need a cap on the power of 2 or else the backoff period
+#   2^N * (milliseconds per hour)
+# will overflow when calcuated within the database. We ensure overflow does not occur
+# by checking that the largest backoff period fits in a 32-bit signed integer.
+_LONGEST_BACKOFF_PERIOD_MILLISECONDS = (
+    (2 ** BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS)
+    * BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_MILLISECONDS
+)
+assert 0 < _LONGEST_BACKOFF_PERIOD_MILLISECONDS <= ((2 ** 31) - 1)
 
 
 # All the info we need while iterating the DAG while backfilling

From 6314b16e27a89e57d0b7cfd0fd4e2ce995c02f83 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 29 Sep 2022 20:45:10 +0100
Subject: [PATCH 10/13] Another comment tweak

---
 synapse/storage/databases/main/event_federation.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/synapse/storage/databases/main/event_federation.py b/synapse/storage/databases/main/event_federation.py
index ffa92c650bb0..e136af5e3729 100644
--- a/synapse/storage/databases/main/event_federation.py
+++ b/synapse/storage/databases/main/event_federation.py
@@ -81,7 +81,8 @@
 # Each successive backoff period is twice as long as the last. However we cap this
 # period at a maximum of 2^8 = 256 hours: a little over 10 days. (This is the smallest
 # power of 2 which yields a maximum backoff period of at least 7 days---which was the
-# original maximum backoff period.)
+# original maximum backoff period.) Even when we hit this cap, we will continue to
+# make backfill attempts once every 10 days.
 BACKFILL_EVENT_EXPONENTIAL_BACKOFF_MAXIMUM_DOUBLING_STEPS = 8
 BACKFILL_EVENT_EXPONENTIAL_BACKOFF_STEP_MILLISECONDS = int(
     datetime.timedelta(hours=1).total_seconds() * 1000

From 34c3900e2e20853909049be5054902cacb53f939 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 29 Sep 2022 21:11:54 +0100
Subject: [PATCH 11/13] Test common backfill path over insertion events

---
 tests/storage/test_event_federation.py | 80 +++++++++++++-------------
 1 file changed, 41 insertions(+), 39 deletions(-)

diff --git a/tests/storage/test_event_federation.py b/tests/storage/test_event_federation.py
index bfedb3b7317a..68989114a7f6 100644
--- a/tests/storage/test_event_federation.py
+++ b/tests/storage/test_event_federation.py
@@ -860,6 +860,47 @@ def test_get_backfill_points_in_room_attempted_event_retry_after_backoff_duratio
             backfill_event_ids, ["b6", "b5", "b4", "2", "b3", "b2", "b1"]
         )
 
+    def test_repeated_get_backfill_points_in_room_succeed(self) -> None:
+        """
+        A test that reproduces #13929 (Postgres only).
+
+        We should be able to backoff repeatedly, even if the backoff formula would tell
+        us to wait for more seconds than can be expressed in a 32 bit signed int.
+        """
+        setup_info = self._setup_room_for_backfill_tests()
+        room_id = setup_info.room_id
+
+        # Pretend that we have tried and failed 10 times to backfill event b1.
+        for _ in range(10):
+            self.get_success(
+                self.store.record_event_failed_pull_attempt(
+                    room_id, "b1", "fake cause"
+                )
+            )
+
+        # If the backoff periods grow without limit:
+        # After the first failed attempt, we would have backed off for 1 << 1 = 2 hours.
+        # After the second failed attempt we would have backed off for 1 << 2 = 4 hours,
+        # so after the 10th failed attempt we should backoff for 1 << 10 == 1024 hours.
+        # Wait 1100 hours just so we have a nice round number.
+        self.reactor.advance(datetime.timedelta(hours=1100).total_seconds())
+
+        # 1024 hours in milliseconds is 1024 * 3600000, which exceeds the largest 32 bit
+        # signed integer. The bug we're reproducing is that this overflow causes an
+        # error in postgres preventing us from fetching a set of backwards extremities
+        # to retry fetching.
+        backfill_points = self.get_success(
+            self.store.get_backfill_points_in_room(room_id)
+        )
+
+        # We should aim to fetch all backoff points: b1's latest backoff period has
+        # expired, and we haven't tried the rest.
+        backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
+        self.assertEqual(
+            backfill_event_ids, ["b6", "b5", "b4", "2", "b3", "b2", "b1"]
+        )
+
+
     def _setup_room_for_insertion_backfill_tests(self) -> _BackfillSetupInfo:
         """
         Sets up a room with various insertion event backward extremities to test
@@ -1049,45 +1090,6 @@ def test_get_insertion_event_backward_extremities_in_room_attempted_event_retry_
             backfill_event_ids, ["insertion_eventB", "insertion_eventA"]
         )
 
-    def test_repeated_backfill_failures_are_retried_appropriately(self) -> None:
-        """
-        A test that reproduces #13929 (Postgres only).
-
-        We should be able to backoff repeatedly, even if the backoff formula would tell
-        us to wait for more seconds than can be expressed in a 32 bit signed int.
-        """
-        setup_info = self._setup_room_for_insertion_backfill_tests()
-        room_id = setup_info.room_id
-
-        # Pretend that we have tried and failed 10 times to backfill event A.
-        for _ in range(10):
-            self.get_success(
-                self.store.record_event_failed_pull_attempt(
-                    room_id, "insertion_eventA", "fake cause"
-                )
-            )
-
-        # If the backoff periods grow without limit:
-        # After the first failed attempt, we would have backed off for 1 << 1 = 2 hours.
-        # After the second failed attempt we would have backed off for 1 << 2 = 4 hours,
-        # so after the 10th failed attempt we should backoff for 1 << 10 == 1024 hours.
-        # Wait 1100 hours just so we have a nice round number.
-        self.reactor.advance(datetime.timedelta(hours=1100).total_seconds())
-
-        # 1024 hours in milliseconds is 1024 * 3600000, which exceeds the largest 32 bit
-        # signed integer. The bug we're reproducing is that this overflow causes an
-        # error in postgres preventing us from fetching a set of backwards extremities
-        # to retry fetching.
-        backfill_points = self.get_success(
-            self.store.get_insertion_event_backward_extremities_in_room(room_id)
-        )
-
-        # We should aim to fetch A (the backoff period has expired) and to fetch B
-        # (we haven't tried to fetch it yet).
-        backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
-        self.assertEqual(
-            backfill_event_ids, ["insertion_eventB", "insertion_eventA"]
-        )
 
 
 @attr.s

From 20204e586066d7cb3d1641565c61cc6af5635f39 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 29 Sep 2022 22:29:12 +0100
Subject: [PATCH 12/13] Tweak test description

Co-authored-by: Eric Eastwood <erice@element.io>
---
 tests/storage/test_event_federation.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/storage/test_event_federation.py b/tests/storage/test_event_federation.py
index c400d8b8bd04..20b1ffa6fd97 100644
--- a/tests/storage/test_event_federation.py
+++ b/tests/storage/test_event_federation.py
@@ -871,12 +871,14 @@ def test_get_backfill_points_in_room_attempted_event_retry_after_backoff_duratio
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
         self.assertEqual(backfill_event_ids, ["b3", "b2", "b1"])
 
-    def test_repeated_get_backfill_points_in_room_succeed(self) -> None:
+    def test_get_backfill_points_in_room_works_after_many_failed_pull_attempts_that_could_naively_overflow(self) -> None:
         """
         A test that reproduces #13929 (Postgres only).
 
-        We should be able to backoff repeatedly, even if the backoff formula would tell
-        us to wait for more seconds than can be expressed in a 32 bit signed int.
+        Test to make sure we can still get backfill points after many failed pull
+        attempts that cause us to backoff to the limit. Even if the backoff formula
+        would tell us to wait for more seconds than can be expressed in a 32 bit
+        signed int.
         """
         setup_info = self._setup_room_for_backfill_tests()
         room_id = setup_info.room_id

From 583029080f7b40de48384f62ae17c36312ebd646 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 29 Sep 2022 22:37:21 +0100
Subject: [PATCH 13/13] Fix the test!!

---
 tests/storage/test_event_federation.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/storage/test_event_federation.py b/tests/storage/test_event_federation.py
index 20b1ffa6fd97..59b891090726 100644
--- a/tests/storage/test_event_federation.py
+++ b/tests/storage/test_event_federation.py
@@ -871,7 +871,9 @@ def test_get_backfill_points_in_room_attempted_event_retry_after_backoff_duratio
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
         self.assertEqual(backfill_event_ids, ["b3", "b2", "b1"])
 
-    def test_get_backfill_points_in_room_works_after_many_failed_pull_attempts_that_could_naively_overflow(self) -> None:
+    def test_get_backfill_points_in_room_works_after_many_failed_pull_attempts_that_could_naively_overflow(
+        self,
+    ) -> None:
         """
         A test that reproduces #13929 (Postgres only).
 
@@ -902,13 +904,13 @@ def test_get_backfill_points_in_room_works_after_many_failed_pull_attempts_that_
         # error in postgres preventing us from fetching a set of backwards extremities
         # to retry fetching.
         backfill_points = self.get_success(
-            self.store.get_backfill_points_in_room(room_id, depth_map["b1"], limit=100)
+            self.store.get_backfill_points_in_room(room_id, depth_map["A"], limit=100)
         )
 
         # We should aim to fetch all backoff points: b1's latest backoff period has
         # expired, and we haven't tried the rest.
         backfill_event_ids = [backfill_point[0] for backfill_point in backfill_points]
-        self.assertEqual(backfill_event_ids, ["b6", "b5", "b4", "2", "b3", "b2", "b1"])
+        self.assertEqual(backfill_event_ids, ["b3", "b2", "b1"])
 
     def _setup_room_for_insertion_backfill_tests(self) -> _BackfillSetupInfo:
         """
