From 77055dba92af80cdfdffc30fd3084ecd24902c2e Mon Sep 17 00:00:00 2001
From: Amber Brown <hawkowl@atleastfornow.net>
Date: Tue, 4 Sep 2018 02:21:48 +1000
Subject: Fix tests on postgresql (#3740)

---
 tests/storage/test_monthly_active_users.py | 138 ++++++++++++++++-------------
 1 file changed, 74 insertions(+), 64 deletions(-)

(limited to 'tests/storage/test_monthly_active_users.py')

diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py
index f2ed866ae7..2036287288 100644
--- a/tests/storage/test_monthly_active_users.py
+++ b/tests/storage/test_monthly_active_users.py
@@ -13,25 +13,22 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from twisted.internet import defer
-
-import tests.unittest
-import tests.utils
-from tests.utils import setup_test_homeserver
+from tests.unittest import HomeserverTestCase
 
 FORTY_DAYS = 40 * 24 * 60 * 60
 
 
-class MonthlyActiveUsersTestCase(tests.unittest.TestCase):
-    def __init__(self, *args, **kwargs):
-        super(MonthlyActiveUsersTestCase, self).__init__(*args, **kwargs)
+class MonthlyActiveUsersTestCase(HomeserverTestCase):
+    def make_homeserver(self, reactor, clock):
+
+        hs = self.setup_test_homeserver()
+        self.store = hs.get_datastore()
+
+        # Advance the clock a bit
+        reactor.advance(FORTY_DAYS)
 
-    @defer.inlineCallbacks
-    def setUp(self):
-        self.hs = yield setup_test_homeserver(self.addCleanup)
-        self.store = self.hs.get_datastore()
+        return hs
 
-    @defer.inlineCallbacks
     def test_initialise_reserved_users(self):
         self.hs.config.max_mau_value = 5
         user1 = "@user1:server"
@@ -44,88 +41,101 @@ class MonthlyActiveUsersTestCase(tests.unittest.TestCase):
         ]
         user_num = len(threepids)
 
-        yield self.store.register(user_id=user1, token="123", password_hash=None)
-
-        yield self.store.register(user_id=user2, token="456", password_hash=None)
+        self.store.register(user_id=user1, token="123", password_hash=None)
+        self.store.register(user_id=user2, token="456", password_hash=None)
+        self.pump()
 
         now = int(self.hs.get_clock().time_msec())
-        yield self.store.user_add_threepid(user1, "email", user1_email, now, now)
-        yield self.store.user_add_threepid(user2, "email", user2_email, now, now)
-        yield self.store.initialise_reserved_users(threepids)
+        self.store.user_add_threepid(user1, "email", user1_email, now, now)
+        self.store.user_add_threepid(user2, "email", user2_email, now, now)
+        self.store.initialise_reserved_users(threepids)
+        self.pump()
 
-        active_count = yield self.store.get_monthly_active_count()
+        active_count = self.store.get_monthly_active_count()
 
         # Test total counts
-        self.assertEquals(active_count, user_num)
+        self.assertEquals(self.get_success(active_count), user_num)
 
         # Test user is marked as active
-
-        timestamp = yield self.store.user_last_seen_monthly_active(user1)
-        self.assertTrue(timestamp)
-        timestamp = yield self.store.user_last_seen_monthly_active(user2)
-        self.assertTrue(timestamp)
+        timestamp = self.store.user_last_seen_monthly_active(user1)
+        self.assertTrue(self.get_success(timestamp))
+        timestamp = self.store.user_last_seen_monthly_active(user2)
+        self.assertTrue(self.get_success(timestamp))
 
         # Test that users are never removed from the db.
         self.hs.config.max_mau_value = 0
 
-        self.hs.get_clock().advance_time(FORTY_DAYS)
+        self.reactor.advance(FORTY_DAYS)
 
-        yield self.store.reap_monthly_active_users()
+        self.store.reap_monthly_active_users()
+        self.pump()
 
-        active_count = yield self.store.get_monthly_active_count()
-        self.assertEquals(active_count, user_num)
+        active_count = self.store.get_monthly_active_count()
+        self.assertEquals(self.get_success(active_count), user_num)
 
         # Test that regalar users are removed from the db
         ru_count = 2
-        yield self.store.upsert_monthly_active_user("@ru1:server")
-        yield self.store.upsert_monthly_active_user("@ru2:server")
-        active_count = yield self.store.get_monthly_active_count()
+        self.store.upsert_monthly_active_user("@ru1:server")
+        self.store.upsert_monthly_active_user("@ru2:server")
+        self.pump()
 
-        self.assertEqual(active_count, user_num + ru_count)
+        active_count = self.store.get_monthly_active_count()
+        self.assertEqual(self.get_success(active_count), user_num + ru_count)
         self.hs.config.max_mau_value = user_num
-        yield self.store.reap_monthly_active_users()
+        self.store.reap_monthly_active_users()
+        self.pump()
 
-        active_count = yield self.store.get_monthly_active_count()
-        self.assertEquals(active_count, user_num)
+        active_count = self.store.get_monthly_active_count()
+        self.assertEquals(self.get_success(active_count), user_num)
 
-    @defer.inlineCallbacks
     def test_can_insert_and_count_mau(self):
-        count = yield self.store.get_monthly_active_count()
-        self.assertEqual(0, count)
+        count = self.store.get_monthly_active_count()
+        self.assertEqual(0, self.get_success(count))
 
-        yield self.store.upsert_monthly_active_user("@user:server")
-        count = yield self.store.get_monthly_active_count()
+        self.store.upsert_monthly_active_user("@user:server")
+        self.pump()
 
-        self.assertEqual(1, count)
+        count = self.store.get_monthly_active_count()
+        self.assertEqual(1, self.get_success(count))
 
-    @defer.inlineCallbacks
     def test_user_last_seen_monthly_active(self):
         user_id1 = "@user1:server"
         user_id2 = "@user2:server"
         user_id3 = "@user3:server"
 
-        result = yield self.store.user_last_seen_monthly_active(user_id1)
-        self.assertFalse(result == 0)
-        yield self.store.upsert_monthly_active_user(user_id1)
-        yield self.store.upsert_monthly_active_user(user_id2)
-        result = yield self.store.user_last_seen_monthly_active(user_id1)
-        self.assertTrue(result > 0)
-        result = yield self.store.user_last_seen_monthly_active(user_id3)
-        self.assertFalse(result == 0)
+        result = self.store.user_last_seen_monthly_active(user_id1)
+        self.assertFalse(self.get_success(result) == 0)
+
+        self.store.upsert_monthly_active_user(user_id1)
+        self.store.upsert_monthly_active_user(user_id2)
+        self.pump()
+
+        result = self.store.user_last_seen_monthly_active(user_id1)
+        self.assertGreater(self.get_success(result), 0)
+
+        result = self.store.user_last_seen_monthly_active(user_id3)
+        self.assertNotEqual(self.get_success(result), 0)
 
-    @defer.inlineCallbacks
     def test_reap_monthly_active_users(self):
         self.hs.config.max_mau_value = 5
         initial_users = 10
         for i in range(initial_users):
-            yield self.store.upsert_monthly_active_user("@user%d:server" % i)
-        count = yield self.store.get_monthly_active_count()
-        self.assertTrue(count, initial_users)
-        yield self.store.reap_monthly_active_users()
-        count = yield self.store.get_monthly_active_count()
-        self.assertEquals(count, initial_users - self.hs.config.max_mau_value)
-
-        self.hs.get_clock().advance_time(FORTY_DAYS)
-        yield self.store.reap_monthly_active_users()
-        count = yield self.store.get_monthly_active_count()
-        self.assertEquals(count, 0)
+            self.store.upsert_monthly_active_user("@user%d:server" % i)
+        self.pump()
+
+        count = self.store.get_monthly_active_count()
+        self.assertTrue(self.get_success(count), initial_users)
+
+        self.store.reap_monthly_active_users()
+        self.pump()
+        count = self.store.get_monthly_active_count()
+        self.assertEquals(
+            self.get_success(count), initial_users - self.hs.config.max_mau_value
+        )
+
+        self.reactor.advance(FORTY_DAYS)
+        self.store.reap_monthly_active_users()
+        self.pump()
+
+        count = self.store.get_monthly_active_count()
+        self.assertEquals(self.get_success(count), 0)
-- 
cgit 1.5.1


From 61b05727fa6ba879f507d13ff40c0197916ef795 Mon Sep 17 00:00:00 2001
From: Neil Johnson <neil@fragile.org.uk>
Date: Wed, 5 Sep 2018 22:30:36 +0100
Subject: guest users should not be part of mau total

---
 synapse/storage/monthly_active_users.py    |  7 ++++-
 tests/storage/test_monthly_active_users.py | 44 ++++++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 3 deletions(-)

(limited to 'tests/storage/test_monthly_active_users.py')

diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py
index c7899d7fd2..e34e16c079 100644
--- a/synapse/storage/monthly_active_users.py
+++ b/synapse/storage/monthly_active_users.py
@@ -192,14 +192,19 @@ class MonthlyActiveUsersStore(SQLBaseStore):
         ))
 
     @defer.inlineCallbacks
-    def populate_monthly_active_users(self, user_id):
+    def populate_monthly_active_users(self, user_id, is_guest=False):
         """Checks on the state of monthly active user limits and optionally
         add the user to the monthly active tables
 
         Args:
             user_id(str): the user_id to query
         """
+
         if self.hs.config.limit_usage_by_mau:
+            # Guests should not be included as part of MAU group
+            if is_guest:
+                return
+
             is_trial = yield self.is_trial_user(user_id)
             if is_trial:
                 # we don't track trial users in the MAU table.
diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py
index 2036287288..cfe0a7c77d 100644
--- a/tests/storage/test_monthly_active_users.py
+++ b/tests/storage/test_monthly_active_users.py
@@ -12,6 +12,9 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+from mock import Mock
+
+from twisted.internet import defer
 
 from tests.unittest import HomeserverTestCase
 
@@ -23,7 +26,8 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
 
         hs = self.setup_test_homeserver()
         self.store = hs.get_datastore()
-
+        hs.config.limit_usage_by_mau = True
+        hs.config.max_mau_value = 50
         # Advance the clock a bit
         reactor.advance(FORTY_DAYS)
 
@@ -73,7 +77,7 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
         active_count = self.store.get_monthly_active_count()
         self.assertEquals(self.get_success(active_count), user_num)
 
-        # Test that regalar users are removed from the db
+        # Test that regular users are removed from the db
         ru_count = 2
         self.store.upsert_monthly_active_user("@ru1:server")
         self.store.upsert_monthly_active_user("@ru2:server")
@@ -139,3 +143,39 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
 
         count = self.store.get_monthly_active_count()
         self.assertEquals(self.get_success(count), 0)
+
+    def test_populate_monthly_users_is_guest(self):
+        # Test that guest users are not added to mau list
+        self.store.upsert_monthly_active_user = Mock()
+        self.store.populate_monthly_active_users('user_id', True)
+        self.pump()
+        self.store.upsert_monthly_active_user.assert_not_called()
+
+    def test_populate_monthly_users_should_update(self):
+        self.store.upsert_monthly_active_user = Mock()
+
+        self.store.is_trial_user = Mock(
+            return_value=defer.succeed(False)
+        )
+
+        self.store.user_last_seen_monthly_active = Mock(
+            return_value=defer.succeed(None)
+        )
+        self.store.populate_monthly_active_users('user_id')
+        self.pump()
+        self.store.upsert_monthly_active_user.assert_called_once()
+
+    def test_populate_monthly_users_should_not_update(self):
+        self.store.upsert_monthly_active_user = Mock()
+
+        self.store.is_trial_user = Mock(
+            return_value=defer.succeed(False)
+        )
+        self.store.user_last_seen_monthly_active = Mock(
+            return_value=defer.succeed(
+                self.hs.get_clock().time_msec()
+            )
+        )
+        self.store.populate_monthly_active_users('user_id')
+        self.pump()
+        self.store.upsert_monthly_active_user.assert_not_called()
-- 
cgit 1.5.1


From 84a750e0c36f653b71a06c564154f257c1b7dee3 Mon Sep 17 00:00:00 2001
From: Neil Johnson <neil@matrix.org>
Date: Thu, 6 Sep 2018 17:22:53 +0100
Subject: ensure guests never enter mau list

---
 synapse/storage/monthly_active_users.py    | 7 +++----
 tests/storage/test_client_ips.py           | 4 +---
 tests/storage/test_monthly_active_users.py | 6 +++++-
 tests/utils.py                             | 1 +
 4 files changed, 10 insertions(+), 8 deletions(-)

(limited to 'tests/storage/test_monthly_active_users.py')

diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py
index e34e16c079..b890c152db 100644
--- a/synapse/storage/monthly_active_users.py
+++ b/synapse/storage/monthly_active_users.py
@@ -192,7 +192,7 @@ class MonthlyActiveUsersStore(SQLBaseStore):
         ))
 
     @defer.inlineCallbacks
-    def populate_monthly_active_users(self, user_id, is_guest=False):
+    def populate_monthly_active_users(self, user_id):
         """Checks on the state of monthly active user limits and optionally
         add the user to the monthly active tables
 
@@ -201,13 +201,12 @@ class MonthlyActiveUsersStore(SQLBaseStore):
         """
 
         if self.hs.config.limit_usage_by_mau:
-            # Guests should not be included as part of MAU group
+            # Trial users and guests should not be included as part of MAU group
+            is_guest = yield self.is_guest(user_id)
             if is_guest:
                 return
-
             is_trial = yield self.is_trial_user(user_id)
             if is_trial:
-                # we don't track trial users in the MAU table.
                 return
 
             last_seen_timestamp = yield self.user_last_seen_monthly_active(user_id)
diff --git a/tests/storage/test_client_ips.py b/tests/storage/test_client_ips.py
index c2e88bdbaf..c9b02a062b 100644
--- a/tests/storage/test_client_ips.py
+++ b/tests/storage/test_client_ips.py
@@ -101,13 +101,11 @@ class ClientIpStoreTestCase(tests.unittest.TestCase):
         self.hs.config.limit_usage_by_mau = True
         self.hs.config.max_mau_value = 50
         user_id = "@user:server"
+        yield self.store.register(user_id=user_id, token="123", password_hash=None)
 
         active = yield self.store.user_last_seen_monthly_active(user_id)
         self.assertFalse(active)
 
-        yield self.store.insert_client_ip(
-            user_id, "access_token", "ip", "user_agent", "device_id"
-        )
         yield self.store.insert_client_ip(
             user_id, "access_token", "ip", "user_agent", "device_id"
         )
diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py
index cfe0a7c77d..ccfc21b7fc 100644
--- a/tests/storage/test_monthly_active_users.py
+++ b/tests/storage/test_monthly_active_users.py
@@ -146,8 +146,12 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
 
     def test_populate_monthly_users_is_guest(self):
         # Test that guest users are not added to mau list
+        user_id = "user_id"
+        self.store.register(
+            user_id=user_id, token="123", password_hash=None, make_guest=True
+        )
         self.store.upsert_monthly_active_user = Mock()
-        self.store.populate_monthly_active_users('user_id', True)
+        self.store.populate_monthly_active_users(user_id)
         self.pump()
         self.store.upsert_monthly_active_user.assert_not_called()
 
diff --git a/tests/utils.py b/tests/utils.py
index 114470d641..f24bd81bf6 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -147,6 +147,7 @@ def setup_test_homeserver(
         config.hs_disabled_message = ""
         config.hs_disabled_limit_type = ""
         config.max_mau_value = 50
+        config.mau_trial_days = 0
         config.mau_limits_reserved_threepids = []
         config.admin_contact = None
         config.rc_messages_per_second = 10000
-- 
cgit 1.5.1


From 0ddf48672421aa64920342b959dc77432c869889 Mon Sep 17 00:00:00 2001
From: Neil Johnson <neil@matrix.org>
Date: Wed, 12 Sep 2018 11:58:52 +0100
Subject: expose number of real reserved users

---
 synapse/app/homeserver.py                  | 10 +++++++---
 synapse/storage/monthly_active_users.py    | 17 ++++++++++++++++
 tests/storage/test_monthly_active_users.py | 31 ++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 3 deletions(-)

(limited to 'tests/storage/test_monthly_active_users.py')

diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py
index 3eb5b663de..e6372bcec2 100755
--- a/synapse/app/homeserver.py
+++ b/synapse/app/homeserver.py
@@ -307,6 +307,7 @@ class SynapseHomeServer(HomeServer):
 # Gauges to expose monthly active user control metrics
 current_mau_gauge = Gauge("synapse_admin_mau:current", "Current MAU")
 max_mau_gauge = Gauge("synapse_admin_mau:max", "MAU Limit")
+reserved_mau_gauge = Gauge("synapse_admin_mau:reserved", "Reserved real MAU users")
 
 
 def setup(config_options):
@@ -531,10 +532,13 @@ def run(hs):
 
     @defer.inlineCallbacks
     def generate_monthly_active_users():
-        count = 0
+        current_mau_count = 0
+        reserved_mau_count = 0
         if hs.config.limit_usage_by_mau:
-            count = yield hs.get_datastore().get_monthly_active_count()
-        current_mau_gauge.set(float(count))
+            current_mau_count = yield hs.get_datastore().get_monthly_active_count()
+            reserved_mau_count = yield hs.get_datastore().get_reserved_real_user_account()
+        current_mau_gauge.set(float(current_mau_count))
+        reserved_mau_gauge.set(float(reserved_mau_count))
         max_mau_gauge.set(float(hs.config.max_mau_value))
 
     hs.get_datastore().initialise_reserved_users(
diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py
index b890c152db..53d125d305 100644
--- a/synapse/storage/monthly_active_users.py
+++ b/synapse/storage/monthly_active_users.py
@@ -146,6 +146,23 @@ class MonthlyActiveUsersStore(SQLBaseStore):
             return count
         return self.runInteraction("count_users", _count_users)
 
+    @defer.inlineCallbacks
+    def get_reserved_real_user_account(self):
+        """Of the reserved threepids defined in config, how many are associated
+        with registered users?
+
+        Returns:
+            Defered[int]: Number of real reserved users
+        """
+        count = 0
+        for tp in self.hs.config.mau_limits_reserved_threepids:
+            user_id = yield self.hs.get_datastore().get_user_id_by_threepid(
+                tp["medium"], tp["address"]
+            )
+            if user_id:
+                count = count + 1
+        defer.returnValue(count)
+
     @defer.inlineCallbacks
     def upsert_monthly_active_user(self, user_id):
         """
diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py
index ccfc21b7fc..662f2ed845 100644
--- a/tests/storage/test_monthly_active_users.py
+++ b/tests/storage/test_monthly_active_users.py
@@ -183,3 +183,34 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
         self.store.populate_monthly_active_users('user_id')
         self.pump()
         self.store.upsert_monthly_active_user.assert_not_called()
+
+    def test_get_reserved_real_user_account(self):
+        # Test no reserved users, or reserved threepids
+        count = self.store.get_reserved_real_user_account()
+        self.assertEquals(self.get_success(count), 0)
+        # Test reserved users but no registered users
+
+        user1 = '@user1:example.com'
+        user2 = '@user2:example.com'
+        user1_email = 'user1@example.com'
+        user2_email = 'user2@example.com'
+        threepids = [
+            {'medium': 'email', 'address': user1_email},
+            {'medium': 'email', 'address': user2_email},
+        ]
+        self.hs.config.mau_limits_reserved_threepids = threepids
+        self.store.initialise_reserved_users(threepids)
+        self.pump()
+        count = self.store.get_reserved_real_user_account()
+        self.assertEquals(self.get_success(count), 0)
+
+        # Test reserved registed users
+        self.store.register(user_id=user1, token="123", password_hash=None)
+        self.store.register(user_id=user2, token="456", password_hash=None)
+        self.pump()
+
+        now = int(self.hs.get_clock().time_msec())
+        self.store.user_add_threepid(user1, "email", user1_email, now, now)
+        self.store.user_add_threepid(user2, "email", user2_email, now, now)
+        count = self.store.get_reserved_real_user_account()
+        self.assertEquals(self.get_success(count), len(threepids))
-- 
cgit 1.5.1


From 8decd6233dabd87160794949ffd95282e60ab01e Mon Sep 17 00:00:00 2001
From: Neil Johnson <neil@matrix.org>
Date: Wed, 12 Sep 2018 16:22:15 +0100
Subject: improve naming

---
 changelog.d/3846.feature                   |  2 +-
 synapse/app/homeserver.py                  | 14 +++++++++-----
 synapse/storage/monthly_active_users.py    |  2 +-
 tests/storage/test_monthly_active_users.py |  6 +++---
 4 files changed, 14 insertions(+), 10 deletions(-)

(limited to 'tests/storage/test_monthly_active_users.py')

diff --git a/changelog.d/3846.feature b/changelog.d/3846.feature
index dc9e9d86cc..453c11d3f8 100644
--- a/changelog.d/3846.feature
+++ b/changelog.d/3846.feature
@@ -1 +1 @@
-create synapse_admin_mau:reserved metric to expose number of real reaserved users 
+Add synapse_admin_mau:registered_reserved_users metric to expose number of real reaserved users 
diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py
index e6372bcec2..ac97e19649 100755
--- a/synapse/app/homeserver.py
+++ b/synapse/app/homeserver.py
@@ -307,7 +307,10 @@ class SynapseHomeServer(HomeServer):
 # Gauges to expose monthly active user control metrics
 current_mau_gauge = Gauge("synapse_admin_mau:current", "Current MAU")
 max_mau_gauge = Gauge("synapse_admin_mau:max", "MAU Limit")
-reserved_mau_gauge = Gauge("synapse_admin_mau:reserved", "Reserved real MAU users")
+registered_reserved_users_mau_gauge = Gauge(
+    "synapse_admin_mau:registered_reserved_users",
+    "Registered users with reserved threepids"
+)
 
 
 def setup(config_options):
@@ -533,12 +536,13 @@ def run(hs):
     @defer.inlineCallbacks
     def generate_monthly_active_users():
         current_mau_count = 0
-        reserved_mau_count = 0
+        reserved_count = 0
+        store = hs.get_datastore()
         if hs.config.limit_usage_by_mau:
-            current_mau_count = yield hs.get_datastore().get_monthly_active_count()
-            reserved_mau_count = yield hs.get_datastore().get_reserved_real_user_account()
+            current_mau_count = yield store.get_monthly_active_count()
+            reserved_count = yield store.get_registered_reserved_users_count()
         current_mau_gauge.set(float(current_mau_count))
-        reserved_mau_gauge.set(float(reserved_mau_count))
+        registered_reserved_users_mau_gauge.set(float(reserved_count))
         max_mau_gauge.set(float(hs.config.max_mau_value))
 
     hs.get_datastore().initialise_reserved_users(
diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py
index 53d125d305..59580949f1 100644
--- a/synapse/storage/monthly_active_users.py
+++ b/synapse/storage/monthly_active_users.py
@@ -147,7 +147,7 @@ class MonthlyActiveUsersStore(SQLBaseStore):
         return self.runInteraction("count_users", _count_users)
 
     @defer.inlineCallbacks
-    def get_reserved_real_user_account(self):
+    def get_registered_reserved_users_count(self):
         """Of the reserved threepids defined in config, how many are associated
         with registered users?
 
diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py
index 662f2ed845..686f12a0dc 100644
--- a/tests/storage/test_monthly_active_users.py
+++ b/tests/storage/test_monthly_active_users.py
@@ -186,7 +186,7 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
 
     def test_get_reserved_real_user_account(self):
         # Test no reserved users, or reserved threepids
-        count = self.store.get_reserved_real_user_account()
+        count = self.store.get_registered_reserved_users_count()
         self.assertEquals(self.get_success(count), 0)
         # Test reserved users but no registered users
 
@@ -201,7 +201,7 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
         self.hs.config.mau_limits_reserved_threepids = threepids
         self.store.initialise_reserved_users(threepids)
         self.pump()
-        count = self.store.get_reserved_real_user_account()
+        count = self.store.get_registered_reserved_users_count()
         self.assertEquals(self.get_success(count), 0)
 
         # Test reserved registed users
@@ -212,5 +212,5 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
         now = int(self.hs.get_clock().time_msec())
         self.store.user_add_threepid(user1, "email", user1_email, now, now)
         self.store.user_add_threepid(user2, "email", user2_email, now, now)
-        count = self.store.get_reserved_real_user_account()
+        count = self.store.get_registered_reserved_users_count()
         self.assertEquals(self.get_success(count), len(threepids))
-- 
cgit 1.5.1


From 6105c6101fa0f4560daf7d23dedcfd217530b02a Mon Sep 17 00:00:00 2001
From: Neil Johnson <neil@matrix.org>
Date: Tue, 23 Oct 2018 15:24:58 +0100
Subject: fix race condiftion in calling initialise_reserved_users

---
 changelog.d/4081.bugfix                    |  2 ++
 synapse/app/homeserver.py                  |  8 ------
 synapse/storage/monthly_active_users.py    | 46 +++++++++++++++++++++---------
 synapse/storage/registration.py            | 16 ++++++++---
 tests/storage/test_monthly_active_users.py | 10 +++++--
 5 files changed, 55 insertions(+), 27 deletions(-)
 create mode 100644 changelog.d/4081.bugfix

(limited to 'tests/storage/test_monthly_active_users.py')

diff --git a/changelog.d/4081.bugfix b/changelog.d/4081.bugfix
new file mode 100644
index 0000000000..f275acb61c
--- /dev/null
+++ b/changelog.d/4081.bugfix
@@ -0,0 +1,2 @@
+Fix race condition in populating reserved users
+
diff --git a/synapse/app/homeserver.py b/synapse/app/homeserver.py
index 0b85b377e3..593e1e75db 100755
--- a/synapse/app/homeserver.py
+++ b/synapse/app/homeserver.py
@@ -553,14 +553,6 @@ def run(hs):
             generate_monthly_active_users,
         )
 
-    # XXX is this really supposed to be a background process? it looks
-    # like it needs to complete before some of the other stuff runs.
-    run_as_background_process(
-        "initialise_reserved_users",
-        hs.get_datastore().initialise_reserved_users,
-        hs.config.mau_limits_reserved_threepids,
-    )
-
     start_generate_monthly_active_users()
     if hs.config.limit_usage_by_mau:
         clock.looping_call(start_generate_monthly_active_users, 5 * 60 * 1000)
diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py
index 0fe8c8e24c..26e577814a 100644
--- a/synapse/storage/monthly_active_users.py
+++ b/synapse/storage/monthly_active_users.py
@@ -33,19 +33,28 @@ class MonthlyActiveUsersStore(SQLBaseStore):
         self._clock = hs.get_clock()
         self.hs = hs
         self.reserved_users = ()
+        self.initialise_reserved_users(
+            dbconn.cursor(), hs.config.mau_limits_reserved_threepids
+        )
 
-    @defer.inlineCallbacks
-    def initialise_reserved_users(self, threepids):
-        store = self.hs.get_datastore()
+    def initialise_reserved_users(self, txn, threepids):
+        """
+        Ensures that reserved threepids are accounted for in the MAU table, should
+        be called on start up.
+
+        Arguments:
+            threepids []: List of threepid dicts to reserve
+        """
         reserved_user_list = []
 
         # Do not add more reserved users than the total allowable number
         for tp in threepids[:self.hs.config.max_mau_value]:
-            user_id = yield store.get_user_id_by_threepid(
+            user_id = self.get_user_id_by_threepid_txn(
+                txn,
                 tp["medium"], tp["address"]
             )
             if user_id:
-                yield self.upsert_monthly_active_user(user_id)
+                self.upsert_monthly_active_user_txn(txn, user_id)
                 reserved_user_list.append(user_id)
             else:
                 logger.warning(
@@ -55,8 +64,7 @@ class MonthlyActiveUsersStore(SQLBaseStore):
 
     @defer.inlineCallbacks
     def reap_monthly_active_users(self):
-        """
-        Cleans out monthly active user table to ensure that no stale
+        """Cleans out monthly active user table to ensure that no stale
         entries exist.
 
         Returns:
@@ -165,19 +173,33 @@ class MonthlyActiveUsersStore(SQLBaseStore):
 
     @defer.inlineCallbacks
     def upsert_monthly_active_user(self, user_id):
+        """Updates or inserts monthly active user member
+        Arguments:
+            user_id (str): user to add/update
+        """
+        is_insert = yield self.runInteraction(
+            "upsert_monthly_active_user", self.upsert_monthly_active_user_txn,
+            user_id
+        )
+        if is_insert:
+            self.user_last_seen_monthly_active.invalidate((user_id,))
+            self.get_monthly_active_count.invalidate(())
+
+    def upsert_monthly_active_user_txn(self, txn, user_id):
         """
             Updates or inserts monthly active user member
             Arguments:
+                txn (cursor):
                 user_id (str): user to add/update
-            Deferred[bool]: True if a new entry was created, False if an
+            bool: True if a new entry was created, False if an
                 existing one was updated.
         """
         # Am consciously deciding to lock the table on the basis that is ought
         # never be a big table and alternative approaches (batching multiple
         # upserts into a single txn) introduced a lot of extra complexity.
         # See https://github.com/matrix-org/synapse/issues/3854 for more
-        is_insert = yield self._simple_upsert(
-            desc="upsert_monthly_active_user",
+        is_insert = self._simple_upsert_txn(
+            txn,
             table="monthly_active_users",
             keyvalues={
                 "user_id": user_id,
@@ -186,9 +208,7 @@ class MonthlyActiveUsersStore(SQLBaseStore):
                 "timestamp": int(self._clock.time_msec()),
             },
         )
-        if is_insert:
-            self.user_last_seen_monthly_active.invalidate((user_id,))
-            self.get_monthly_active_count.invalidate(())
+        return is_insert
 
     @cached(num_args=1)
     def user_last_seen_monthly_active(self, user_id):
diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py
index 26b429e307..01931f29cc 100644
--- a/synapse/storage/registration.py
+++ b/synapse/storage/registration.py
@@ -474,17 +474,25 @@ class RegistrationStore(RegistrationWorkerStore,
 
     @defer.inlineCallbacks
     def get_user_id_by_threepid(self, medium, address):
-        ret = yield self._simple_select_one(
+        user_id = yield self.runInteraction(
+            "get_user_id_by_threepid", self.get_user_id_by_threepid_txn,
+            medium, address
+        )
+        defer.returnValue(user_id)
+
+    def get_user_id_by_threepid_txn(self, txn, medium, address):
+        ret = self._simple_select_one_txn(
+            txn,
             "user_threepids",
             {
                 "medium": medium,
                 "address": address
             },
-            ['user_id'], True, 'get_user_id_by_threepid'
+            ['user_id'], True
         )
         if ret:
-            defer.returnValue(ret['user_id'])
-        defer.returnValue(None)
+            return ret['user_id']
+        return None
 
     def user_delete_threepid(self, user_id, medium, address):
         return self._simple_delete(
diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py
index 686f12a0dc..0c17745ae7 100644
--- a/tests/storage/test_monthly_active_users.py
+++ b/tests/storage/test_monthly_active_users.py
@@ -52,7 +52,10 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
         now = int(self.hs.get_clock().time_msec())
         self.store.user_add_threepid(user1, "email", user1_email, now, now)
         self.store.user_add_threepid(user2, "email", user2_email, now, now)
-        self.store.initialise_reserved_users(threepids)
+
+        self.store.runInteraction(
+            "initialise", self.store.initialise_reserved_users, threepids
+        )
         self.pump()
 
         active_count = self.store.get_monthly_active_count()
@@ -199,7 +202,10 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
             {'medium': 'email', 'address': user2_email},
         ]
         self.hs.config.mau_limits_reserved_threepids = threepids
-        self.store.initialise_reserved_users(threepids)
+        self.store.runInteraction(
+            "initialise", self.store.initialise_reserved_users, threepids
+        )
+
         self.pump()
         count = self.store.get_registered_reserved_users_count()
         self.assertEquals(self.get_success(count), 0)
-- 
cgit 1.5.1


From ea69a84bbb2fc9c1da6db0384a98f577f3bc95a7 Mon Sep 17 00:00:00 2001
From: Neil Johnson <neil@matrix.org>
Date: Wed, 24 Oct 2018 17:18:08 +0100
Subject: fix style inconsistencies

---
 changelog.d/4081.bugfix                    |  3 ++-
 synapse/storage/monthly_active_users.py    | 43 +++++++++++++++++++-----------
 synapse/storage/registration.py            | 19 +++++++++++++
 tests/storage/test_monthly_active_users.py |  4 +--
 4 files changed, 51 insertions(+), 18 deletions(-)

(limited to 'tests/storage/test_monthly_active_users.py')

diff --git a/changelog.d/4081.bugfix b/changelog.d/4081.bugfix
index 13dad58842..cfe4b3e9d9 100644
--- a/changelog.d/4081.bugfix
+++ b/changelog.d/4081.bugfix
@@ -1 +1,2 @@
-Fix race condition in populating reserved users
+Fix race condition where config defined reserved users were not being added to
+the monthly active user list prior to the homeserver reactor firing up
diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py
index cf15f8c5ba..9a5c3b7eda 100644
--- a/synapse/storage/monthly_active_users.py
+++ b/synapse/storage/monthly_active_users.py
@@ -33,21 +33,23 @@ class MonthlyActiveUsersStore(SQLBaseStore):
         self._clock = hs.get_clock()
         self.hs = hs
         self.reserved_users = ()
-        self.initialise_reserved_users(
-            dbconn.cursor(), hs.config.mau_limits_reserved_threepids
+        # Do not add more reserved users than the total allowable number
+        self._initialise_reserved_users(
+            dbconn.cursor(),
+            hs.config.mau_limits_reserved_threepids[:self.hs.config.max_mau_value],
         )
 
-    def initialise_reserved_users(self, txn, threepids):
+    def _initialise_reserved_users(self, txn, threepids):
         """Ensures that reserved threepids are accounted for in the MAU table, should
         be called on start up.
 
-        Arguments:
-            threepids []: List of threepid dicts to reserve
+        Args:
+            txn (cursor):
+            threepids (list[dict]): List of threepid dicts to reserve
         """
         reserved_user_list = []
 
-        # Do not add more reserved users than the total allowable number
-        for tp in threepids[:self.hs.config.max_mau_value]:
+        for tp in threepids:
             user_id = self.get_user_id_by_threepid_txn(
                 txn,
                 tp["medium"], tp["address"]
@@ -172,26 +174,36 @@ class MonthlyActiveUsersStore(SQLBaseStore):
 
     @defer.inlineCallbacks
     def upsert_monthly_active_user(self, user_id):
-        """Updates or inserts monthly active user member
-        Arguments:
+        """Updates or inserts the user into the monthly active user table, which
+        is used to track the current MAU usage of the server
+
+        Args:
             user_id (str): user to add/update
         """
         is_insert = yield self.runInteraction(
             "upsert_monthly_active_user", self.upsert_monthly_active_user_txn,
             user_id
         )
+        # Considered pushing cache invalidation down into txn method, but
+        # did not because txn is not a LoggingTransaction. This means I could not
+        # call txn.call_after(). Therefore cache is altered in background thread
+        # and calls from elsewhere to user_last_seen_monthly_active and
+        # get_monthly_active_count fail with ValueError in
+        #  synapse/util/caches/descriptors.py#check_thread
         if is_insert:
             self.user_last_seen_monthly_active.invalidate((user_id,))
             self.get_monthly_active_count.invalidate(())
 
     def upsert_monthly_active_user_txn(self, txn, user_id):
-        """
-            Updates or inserts monthly active user member
-            Arguments:
-                txn (cursor):
-                user_id (str): user to add/update
+        """Updates or inserts monthly active user member
+
+        Args:
+            txn (cursor):
+            user_id (str): user to add/update
+
+        Returns:
             bool: True if a new entry was created, False if an
-                existing one was updated.
+            existing one was updated.
         """
         # Am consciously deciding to lock the table on the basis that is ought
         # never be a big table and alternative approaches (batching multiple
@@ -207,6 +219,7 @@ class MonthlyActiveUsersStore(SQLBaseStore):
                 "timestamp": int(self._clock.time_msec()),
             },
         )
+
         return is_insert
 
     @cached(num_args=1)
diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py
index 0f970850e8..80d76bf9d7 100644
--- a/synapse/storage/registration.py
+++ b/synapse/storage/registration.py
@@ -474,6 +474,15 @@ class RegistrationStore(RegistrationWorkerStore,
 
     @defer.inlineCallbacks
     def get_user_id_by_threepid(self, medium, address):
+        """Returns user id from threepid
+
+        Args:
+            medium (str): threepid medium e.g. email
+            address (str): threepid address e.g. me@example.com
+
+        Returns:
+            Deferred[str|None]: user id or None if no user id/threepid mapping exists
+        """
         user_id = yield self.runInteraction(
             "get_user_id_by_threepid", self.get_user_id_by_threepid_txn,
             medium, address
@@ -481,6 +490,16 @@ class RegistrationStore(RegistrationWorkerStore,
         defer.returnValue(user_id)
 
     def get_user_id_by_threepid_txn(self, txn, medium, address):
+        """Returns user id from threepid
+
+        Args:
+            txn (cursor):
+            medium (str): threepid medium e.g. email
+            address (str): threepid address e.g. me@example.com
+
+        Returns:
+            str|None: user id or None if no user id/threepid mapping exists
+        """
         ret = self._simple_select_one_txn(
             txn,
             "user_threepids",
diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py
index 0c17745ae7..832e379a83 100644
--- a/tests/storage/test_monthly_active_users.py
+++ b/tests/storage/test_monthly_active_users.py
@@ -54,7 +54,7 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
         self.store.user_add_threepid(user2, "email", user2_email, now, now)
 
         self.store.runInteraction(
-            "initialise", self.store.initialise_reserved_users, threepids
+            "initialise", self.store._initialise_reserved_users, threepids
         )
         self.pump()
 
@@ -203,7 +203,7 @@ class MonthlyActiveUsersTestCase(HomeserverTestCase):
         ]
         self.hs.config.mau_limits_reserved_threepids = threepids
         self.store.runInteraction(
-            "initialise", self.store.initialise_reserved_users, threepids
+            "initialise", self.store._initialise_reserved_users, threepids
         )
 
         self.pump()
-- 
cgit 1.5.1