From ad5b4074e1a84b1e50a97144fbf1902ddc89db73 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Fri, 17 May 2019 19:37:31 +0100
Subject: Add startup background job for account validity

If account validity is enabled in the server's configuration, this job will run at startup as a background job and will stick an expiration date to any registered account missing one.
---
 synapse/storage/_base.py | 62 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

(limited to 'synapse/storage/_base.py')

diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py
index 983ce026e1..06d516c9e2 100644
--- a/synapse/storage/_base.py
+++ b/synapse/storage/_base.py
@@ -1,5 +1,7 @@
 # -*- coding: utf-8 -*-
 # Copyright 2014-2016 OpenMarket Ltd
+# Copyright 2017-2018 New Vector Ltd
+# Copyright 2019 The Matrix.org Foundation C.I.C.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -227,6 +229,8 @@ class SQLBaseStore(object):
         # A set of tables that are not safe to use native upserts in.
         self._unsafe_to_upsert_tables = set(UNIQUE_INDEX_BACKGROUND_UPDATES.keys())
 
+        self._account_validity = self.hs.config.account_validity
+
         # We add the user_directory_search table to the blacklist on SQLite
         # because the existing search table does not have an index, making it
         # unsafe to use native upserts.
@@ -243,6 +247,14 @@ class SQLBaseStore(object):
                 self._check_safe_to_upsert,
             )
 
+        if self._account_validity.enabled:
+            self._clock.call_later(
+                0.0,
+                run_as_background_process,
+                "account_validity_set_expiration_dates",
+                self._set_expiration_date_when_missing,
+            )
+
     @defer.inlineCallbacks
     def _check_safe_to_upsert(self):
         """
@@ -275,6 +287,56 @@ class SQLBaseStore(object):
                 self._check_safe_to_upsert,
             )
 
+    @defer.inlineCallbacks
+    def _set_expiration_date_when_missing(self):
+        """
+        Retrieves the list of registered users that don't have an expiration date, and
+        adds an expiration date for each of them.
+        """
+
+        def select_users_with_no_expiration_date_txn(txn):
+            """Retrieves the list of registered users with no expiration date from the
+            database.
+            """
+            sql = (
+                "SELECT users.name FROM users"
+                " LEFT JOIN account_validity ON (users.name = account_validity.user_id)"
+                " WHERE account_validity.user_id is NULL;"
+            )
+            txn.execute(sql, [])
+            return self.cursor_to_dict(txn)
+
+        res = yield self.runInteraction(
+            "get_users_with_no_expiration_date",
+            select_users_with_no_expiration_date_txn,
+        )
+
+        if res:
+            for user in res:
+                self.runInteraction(
+                    "set_expiration_date_for_user_background",
+                    self.set_expiration_date_for_user_txn,
+                    user["name"],
+                )
+
+    def set_expiration_date_for_user_txn(self, txn, user_id):
+        """Sets an expiration date to the account with the given user ID.
+
+        Args:
+             user_id (str): User ID to set an expiration date for.
+        """
+        now_ms = self._clock.time_msec()
+        expiration_ts = now_ms + self._account_validity.period
+        self._simple_insert_txn(
+            txn,
+            "account_validity",
+            values={
+                "user_id": user_id,
+                "expiration_ts_ms": expiration_ts,
+                "email_sent": False,
+            },
+        )
+
     def start_profiling(self):
         self._previous_loop_ts = self._clock.time_msec()
 
-- 
cgit 1.4.1


From 5ceee46c6bd55376ff2188b6e674035d7da95f24 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 21 May 2019 13:38:51 +0100
Subject: Do the select and insert in a single transaction

---
 synapse/storage/_base.py | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

(limited to 'synapse/storage/_base.py')

diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py
index 06d516c9e2..fa6839ceca 100644
--- a/synapse/storage/_base.py
+++ b/synapse/storage/_base.py
@@ -304,21 +304,17 @@ class SQLBaseStore(object):
                 " WHERE account_validity.user_id is NULL;"
             )
             txn.execute(sql, [])
-            return self.cursor_to_dict(txn)
 
-        res = yield self.runInteraction(
+            res = self.cursor_to_dict(txn)
+            if res:
+                for user in res:
+                    self.set_expiration_date_for_user_txn(txn, user["name"])
+
+        yield self.runInteraction(
             "get_users_with_no_expiration_date",
             select_users_with_no_expiration_date_txn,
         )
 
-        if res:
-            for user in res:
-                self.runInteraction(
-                    "set_expiration_date_for_user_background",
-                    self.set_expiration_date_for_user_txn,
-                    user["name"],
-                )
-
     def set_expiration_date_for_user_txn(self, txn, user_id):
         """Sets an expiration date to the account with the given user ID.
 
-- 
cgit 1.4.1


From 52839886d664576831462e033b88e5aba4c019e3 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 28 May 2019 16:47:42 +0100
Subject: Allow configuring a range for the account validity startup job

When enabling the account validity feature, Synapse will look at startup for registered account without an expiration date, and will set one equals to 'now + validity_period' for them. On large servers, it can mean that a large number of users will have the same expiration date, which means that they will all be sent a renewal email at the same time, which isn't ideal.
In order to mitigate this, this PR allows server admins to define a 'max_delta' so that the expiration date is a random value in the [now + validity_period ; now + validity_period + max_delta] range. This allows renewal emails to be progressively sent over a configured period instead of being sent all in one big batch.
---
 synapse/config/registration.py              | 11 +++++++++++
 synapse/storage/_base.py                    | 23 +++++++++++++++++++++--
 tests/rest/client/v2_alpha/test_register.py | 21 +++++++++++++++++++++
 3 files changed, 53 insertions(+), 2 deletions(-)

(limited to 'synapse/storage/_base.py')

diff --git a/synapse/config/registration.py b/synapse/config/registration.py
index 693288f938..b4fd4af368 100644
--- a/synapse/config/registration.py
+++ b/synapse/config/registration.py
@@ -39,6 +39,10 @@ class AccountValidityConfig(Config):
             else:
                 self.renew_email_subject = "Renew your %(app)s account"
 
+            self.startup_job_max_delta = self.parse_duration(
+                config.get("startup_job_max_delta", 0),
+            )
+
         if self.renew_by_email_enabled and "public_baseurl" not in synapse_config:
             raise ConfigError("Can't send renewal emails without 'public_baseurl'")
 
@@ -131,11 +135,18 @@ class RegistrationConfig(Config):
         # after that the validity period changes and Synapse is restarted, the users'
         # expiration dates won't be updated unless their account is manually renewed.
         #
+        # If set, the ``startup_job_max_delta`` optional setting will make the startup job
+        # described above set a random expiration date between t + period  and
+        # t + period + startup_job_max_delta, t being the date and time at which the job
+        # sets the expiration date for a given user. This is useful for server admins that
+        # want to avoid Synapse sending a lot of renewal emails at once.
+        #
         #account_validity:
         #  enabled: True
         #  period: 6w
         #  renew_at: 1w
         #  renew_email_subject: "Renew your %%(app)s account"
+        #  startup_job_max_delta: 2d
 
         # The user must provide all of the below types of 3PID when registering.
         #
diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py
index fa6839ceca..40802fd3dc 100644
--- a/synapse/storage/_base.py
+++ b/synapse/storage/_base.py
@@ -16,6 +16,7 @@
 # limitations under the License.
 import itertools
 import logging
+import random
 import sys
 import threading
 import time
@@ -247,6 +248,8 @@ class SQLBaseStore(object):
                 self._check_safe_to_upsert,
             )
 
+        self.rand = random.SystemRandom()
+
         if self._account_validity.enabled:
             self._clock.call_later(
                 0.0,
@@ -308,21 +311,37 @@ class SQLBaseStore(object):
             res = self.cursor_to_dict(txn)
             if res:
                 for user in res:
-                    self.set_expiration_date_for_user_txn(txn, user["name"])
+                    self.set_expiration_date_for_user_txn(
+                        txn,
+                        user["name"],
+                        use_delta=True,
+                    )
 
         yield self.runInteraction(
             "get_users_with_no_expiration_date",
             select_users_with_no_expiration_date_txn,
         )
 
-    def set_expiration_date_for_user_txn(self, txn, user_id):
+    def set_expiration_date_for_user_txn(self, txn, user_id, use_delta=False):
         """Sets an expiration date to the account with the given user ID.
 
         Args:
              user_id (str): User ID to set an expiration date for.
+             use_delta (bool): If set to False, the expiration date for the user will be
+                now + validity period. If set to True, this expiration date will be a
+                random value in the [now + period; now + period + max_delta] range,
+                max_delta being the configured value for the size of the range, unless
+                delta is 0, in which case it sets it to now + period.
         """
         now_ms = self._clock.time_msec()
         expiration_ts = now_ms + self._account_validity.period
+
+        if use_delta and self._account_validity.startup_job_max_delta:
+            expiration_ts = self.rand.randrange(
+                expiration_ts,
+                expiration_ts + self._account_validity.startup_job_max_delta,
+            )
+
         self._simple_insert_txn(
             txn,
             "account_validity",
diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py
index d4a1d4d50c..7603440fd8 100644
--- a/tests/rest/client/v2_alpha/test_register.py
+++ b/tests/rest/client/v2_alpha/test_register.py
@@ -436,6 +436,7 @@ class AccountValidityBackgroundJobTestCase(unittest.HomeserverTestCase):
 
     def make_homeserver(self, reactor, clock):
         self.validity_period = 10
+        self.max_delta = 10
 
         config = self.default_config()
 
@@ -459,8 +460,28 @@ class AccountValidityBackgroundJobTestCase(unittest.HomeserverTestCase):
         """
         user_id = self.register_user("kermit", "user")
 
+        self.hs.config.account_validity.startup_job_max_delta = 0
+
         now_ms = self.hs.clock.time_msec()
         self.get_success(self.store._set_expiration_date_when_missing())
 
         res = self.get_success(self.store.get_expiration_ts_for_user(user_id))
         self.assertEqual(res, now_ms + self.validity_period)
+
+    def test_background_job_with_max_delta(self):
+        """
+        Tests the same thing as test_background_job, except that it sets the
+        startup_job_max_delta parameter and checks that the expiration date is within the
+        allowed range.
+        """
+        user_id = self.register_user("kermit_delta", "user")
+
+        self.hs.config.account_validity.startup_job_max_delta = self.max_delta
+
+        now_ms = self.hs.clock.time_msec()
+        self.get_success(self.store._set_expiration_date_when_missing())
+
+        res = self.get_success(self.store.get_expiration_ts_for_user(user_id))
+
+        self.assertLessEqual(res, now_ms + self.validity_period + self.delta)
+        self.assertGreaterEqual(res, now_ms + self.validity_period)
-- 
cgit 1.4.1


From 7e8e683754cdc606a1440832d9b1eb47f930ddee Mon Sep 17 00:00:00 2001
From: Erik Johnston <erik@matrix.org>
Date: Wed, 29 May 2019 11:58:32 +0100
Subject: Log actual number of entries deleted

---
 synapse/storage/_base.py  | 12 +++++++++---
 synapse/storage/events.py |  6 ++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

(limited to 'synapse/storage/_base.py')

diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py
index fa6839ceca..3fe827cd43 100644
--- a/synapse/storage/_base.py
+++ b/synapse/storage/_base.py
@@ -1261,7 +1261,8 @@ class SQLBaseStore(object):
             " AND ".join("%s = ?" % (k,) for k in keyvalues),
         )
 
-        return txn.execute(sql, list(keyvalues.values()))
+        txn.execute(sql, list(keyvalues.values()))
+        return txn.rowcount
 
     def _simple_delete_many(self, table, column, iterable, keyvalues, desc):
         return self.runInteraction(
@@ -1280,9 +1281,12 @@ class SQLBaseStore(object):
             column : column name to test for inclusion against `iterable`
             iterable : list
             keyvalues : dict of column names and values to select the rows with
+
+        Returns:
+            int: Number rows deleted
         """
         if not iterable:
-            return
+            return 0
 
         sql = "DELETE FROM %s" % table
 
@@ -1297,7 +1301,9 @@ class SQLBaseStore(object):
 
         if clauses:
             sql = "%s WHERE %s" % (sql, " AND ".join(clauses))
-        return txn.execute(sql, values)
+        txn.execute(sql, values)
+
+        return txn.rowcount
 
     def _get_cache_dict(
         self, db_conn, table, entity_column, stream_column, max_value, limit=100000
diff --git a/synapse/storage/events.py b/synapse/storage/events.py
index a9be143bd5..a9664928ca 100644
--- a/synapse/storage/events.py
+++ b/synapse/storage/events.py
@@ -2476,7 +2476,7 @@ class EventsStore(
 
             logger.info("Deleting up to %d forward extremities", len(to_delete))
 
-            self._simple_delete_many_txn(
+            deleted = self._simple_delete_many_txn(
                 txn=txn,
                 table="event_forward_extremities",
                 column="event_id",
@@ -2484,7 +2484,9 @@ class EventsStore(
                 keyvalues={},
             )
 
-            if to_delete:
+            logger.info("Deleted %d forward extremities", deleted)
+
+            if deleted:
                 # We now need to invalidate the caches of these rooms
                 rows = self._simple_select_many_txn(
                     txn,
-- 
cgit 1.4.1


From 847b9dcd1c9d7d7a43333e85f69dc78471095475 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Fri, 31 May 2019 09:54:46 +0100
Subject: Make max_delta equal to period * 10%

---
 synapse/config/registration.py              | 15 ++++-----------
 synapse/storage/_base.py                    |  7 +++----
 tests/rest/client/v2_alpha/test_register.py | 18 +-----------------
 3 files changed, 8 insertions(+), 32 deletions(-)

(limited to 'synapse/storage/_base.py')

diff --git a/synapse/config/registration.py b/synapse/config/registration.py
index b4fd4af368..1835b4b1f3 100644
--- a/synapse/config/registration.py
+++ b/synapse/config/registration.py
@@ -39,9 +39,7 @@ class AccountValidityConfig(Config):
             else:
                 self.renew_email_subject = "Renew your %(app)s account"
 
-            self.startup_job_max_delta = self.parse_duration(
-                config.get("startup_job_max_delta", 0),
-            )
+            self.startup_job_max_delta = self.period * 10. / 100.
 
         if self.renew_by_email_enabled and "public_baseurl" not in synapse_config:
             raise ConfigError("Can't send renewal emails without 'public_baseurl'")
@@ -133,20 +131,15 @@ class RegistrationConfig(Config):
         # This means that, if a validity period is set, and Synapse is restarted (it will
         # then derive an expiration date from the current validity period), and some time
         # after that the validity period changes and Synapse is restarted, the users'
-        # expiration dates won't be updated unless their account is manually renewed.
-        #
-        # If set, the ``startup_job_max_delta`` optional setting will make the startup job
-        # described above set a random expiration date between t + period  and
-        # t + period + startup_job_max_delta, t being the date and time at which the job
-        # sets the expiration date for a given user. This is useful for server admins that
-        # want to avoid Synapse sending a lot of renewal emails at once.
+        # expiration dates won't be updated unless their account is manually renewed. This
+        # date will be randomly selected within a range [now + period ; now + period + d],
+        # where d is equal to 10% of the validity period.
         #
         #account_validity:
         #  enabled: True
         #  period: 6w
         #  renew_at: 1w
         #  renew_email_subject: "Renew your %%(app)s account"
-        #  startup_job_max_delta: 2d
 
         # The user must provide all of the below types of 3PID when registering.
         #
diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py
index 40802fd3dc..7f944ec717 100644
--- a/synapse/storage/_base.py
+++ b/synapse/storage/_base.py
@@ -329,14 +329,13 @@ class SQLBaseStore(object):
              user_id (str): User ID to set an expiration date for.
              use_delta (bool): If set to False, the expiration date for the user will be
                 now + validity period. If set to True, this expiration date will be a
-                random value in the [now + period; now + period + max_delta] range,
-                max_delta being the configured value for the size of the range, unless
-                delta is 0, in which case it sets it to now + period.
+                random value in the [now + period; now + period + d] range, d being a
+                delta equal to 10% of the validity period.
         """
         now_ms = self._clock.time_msec()
         expiration_ts = now_ms + self._account_validity.period
 
-        if use_delta and self._account_validity.startup_job_max_delta:
+        if use_delta:
             expiration_ts = self.rand.randrange(
                 expiration_ts,
                 expiration_ts + self._account_validity.startup_job_max_delta,
diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py
index 68654e25ab..711628ded1 100644
--- a/tests/rest/client/v2_alpha/test_register.py
+++ b/tests/rest/client/v2_alpha/test_register.py
@@ -436,7 +436,7 @@ class AccountValidityBackgroundJobTestCase(unittest.HomeserverTestCase):
 
     def make_homeserver(self, reactor, clock):
         self.validity_period = 10
-        self.max_delta = 10
+        self.max_delta = self.validity_period * 10. / 100.
 
         config = self.default_config()
 
@@ -453,22 +453,6 @@ class AccountValidityBackgroundJobTestCase(unittest.HomeserverTestCase):
         return self.hs
 
     def test_background_job(self):
-        """
-        Tests whether the account validity startup background job does the right thing,
-        which is sticking an expiration date to every account that doesn't already have
-        one.
-        """
-        user_id = self.register_user("kermit", "user")
-
-        self.hs.config.account_validity.startup_job_max_delta = 0
-
-        now_ms = self.hs.clock.time_msec()
-        self.get_success(self.store._set_expiration_date_when_missing())
-
-        res = self.get_success(self.store.get_expiration_ts_for_user(user_id))
-        self.assertEqual(res, now_ms + self.validity_period)
-
-    def test_background_job_with_max_delta(self):
         """
         Tests the same thing as test_background_job, except that it sets the
         startup_job_max_delta parameter and checks that the expiration date is within the
-- 
cgit 1.4.1


From 4d794dae210ce30e87d8a6b9ee2f9b481cadf539 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Fri, 31 May 2019 11:09:34 +0100
Subject: Move delta from +10% to -10%

---
 synapse/config/registration.py              | 2 +-
 synapse/storage/_base.py                    | 4 ++--
 tests/rest/client/v2_alpha/test_register.py | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

(limited to 'synapse/storage/_base.py')

diff --git a/synapse/config/registration.py b/synapse/config/registration.py
index 4af825a2ab..aad3400819 100644
--- a/synapse/config/registration.py
+++ b/synapse/config/registration.py
@@ -132,7 +132,7 @@ class RegistrationConfig(Config):
         # then derive an expiration date from the current validity period), and some time
         # after that the validity period changes and Synapse is restarted, the users'
         # expiration dates won't be updated unless their account is manually renewed. This
-        # date will be randomly selected within a range [now + period ; now + period + d],
+        # date will be randomly selected within a range [now + period - d ; now + period],
         # where d is equal to 10%% of the validity period.
         #
         #account_validity:
diff --git a/synapse/storage/_base.py b/synapse/storage/_base.py
index 7f944ec717..086318a530 100644
--- a/synapse/storage/_base.py
+++ b/synapse/storage/_base.py
@@ -329,7 +329,7 @@ class SQLBaseStore(object):
              user_id (str): User ID to set an expiration date for.
              use_delta (bool): If set to False, the expiration date for the user will be
                 now + validity period. If set to True, this expiration date will be a
-                random value in the [now + period; now + period + d] range, d being a
+                random value in the [now + period - d ; now + period] range, d being a
                 delta equal to 10% of the validity period.
         """
         now_ms = self._clock.time_msec()
@@ -337,8 +337,8 @@ class SQLBaseStore(object):
 
         if use_delta:
             expiration_ts = self.rand.randrange(
+                expiration_ts - self._account_validity.startup_job_max_delta,
                 expiration_ts,
-                expiration_ts + self._account_validity.startup_job_max_delta,
             )
 
         self._simple_insert_txn(
diff --git a/tests/rest/client/v2_alpha/test_register.py b/tests/rest/client/v2_alpha/test_register.py
index 711628ded1..0cb6a363d6 100644
--- a/tests/rest/client/v2_alpha/test_register.py
+++ b/tests/rest/client/v2_alpha/test_register.py
@@ -467,5 +467,5 @@ class AccountValidityBackgroundJobTestCase(unittest.HomeserverTestCase):
 
         res = self.get_success(self.store.get_expiration_ts_for_user(user_id))
 
-        self.assertLessEqual(res, now_ms + self.validity_period + self.max_delta)
-        self.assertGreaterEqual(res, now_ms + self.validity_period)
+        self.assertGreaterEqual(res, now_ms + self.validity_period - self.max_delta)
+        self.assertLessEqual(res, now_ms + self.validity_period)
-- 
cgit 1.4.1