From 99a623d2095f25909136b60043672a539266e167 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Thu, 29 Sep 2022 17:24:26 -0500
Subject: [PATCH 01/16] Check appservice user interest against the local users
 instead of all users

`get_local_users_in_room` is way more performant since it looks at a single
table (`local_current_membership`) and is looking through way less
data since it only worries about the local users in the room instead
of everyone in the room across the federation.

Fix https://github.com/matrix-org/synapse/issues/13942

Related to:

 - https://github.com/matrix-org/synapse/pull/13605
 - https://github.com/matrix-org/synapse/pull/13608
 - https://github.com/matrix-org/synapse/pull/13606
---
 synapse/appservice/__init__.py               | 10 +++-------
 synapse/handlers/room_member.py              |  2 +-
 synapse/handlers/user_directory.py           |  1 +
 synapse/storage/databases/main/appservice.py | 14 +++++++++++++-
 synapse/storage/databases/main/roommember.py | 19 ++++++++++++-------
 5 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py
index 0dfa00df44c7..71b534b0397f 100644
--- a/synapse/appservice/__init__.py
+++ b/synapse/appservice/__init__.py
@@ -172,15 +172,11 @@ async def _matches_user_in_member_list(
         Returns:
             True if this service would like to know about this room.
         """
-        member_list = await store.get_users_in_room(
-            room_id, on_invalidate=cache_context.invalidate
+        app_service_users_in_room = await store.get_app_service_users_in_room(
+            room_id, self, on_invalidate=cache_context.invalidate
         )
 
-        # check joined member events
-        for user_id in member_list:
-            if self.is_interested_in_user(user_id):
-                return True
-        return False
+        return len(app_service_users_in_room) > 0
 
     def is_interested_in_user(
         self,
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 88158822e009..96cc6eae71be 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -1150,7 +1150,7 @@ async def transfer_room_state_on_room_upgrade(
         logger.info("Transferring room state from %s to %s", old_room_id, room_id)
 
         # Find all local users that were in the old room and copy over each user's state
-        users = await self.store.get_users_in_room(old_room_id)
+        users = await self.store.get_local_users_in_room(old_room_id)
         await self.copy_user_state_on_room_upgrade(old_room_id, room_id, users)
 
         # Add new room to the room directory if the old room was there
diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py
index 8c3c52e1caa6..19df4de43930 100644
--- a/synapse/handlers/user_directory.py
+++ b/synapse/handlers/user_directory.py
@@ -392,6 +392,7 @@ async def _track_user_joined_room(self, room_id: str, user_id: str) -> None:
         if is_public:
             await self.store.add_users_in_public_rooms(room_id, (user_id,))
         else:
+            # TODO: get_local_users_in_room here
             users_in_room = await self.store.get_users_in_room(room_id)
             other_users_in_room = [
                 other
diff --git a/synapse/storage/databases/main/appservice.py b/synapse/storage/databases/main/appservice.py
index 64b70a7b28ee..26fd4fa31a85 100644
--- a/synapse/storage/databases/main/appservice.py
+++ b/synapse/storage/databases/main/appservice.py
@@ -157,7 +157,19 @@ async def get_app_service_users_in_room(
         app_service: "ApplicationService",
         cache_context: _CacheContext,
     ) -> List[str]:
-        users_in_room = await self.get_users_in_room(
+        """
+        Get all users in a room that the appservice controls.
+
+        Args:
+            room_id: The room to check in.
+            app_service: The application service to check interest/control against
+
+        Returns:
+            List of user IDs that the appservice controls.
+        """
+        # We can use `get_local_users_in_room(...)` here because an application
+        # service can only act on behalf of users of the server it's on.
+        users_in_room = await self.get_local_users_in_room(
             room_id, on_invalidate=cache_context.invalidate
         )
         return list(filter(app_service.is_interested_in_user, users_in_room))
diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py
index 982e1f08e30b..8a9e553397b4 100644
--- a/synapse/storage/databases/main/roommember.py
+++ b/synapse/storage/databases/main/roommember.py
@@ -152,13 +152,18 @@ async def get_users_in_room(self, room_id: str) -> List[str]:
         `get_current_hosts_in_room()` and so we can re-use the cache but it's
         not horrible to have here either.
 
-        Uses `m.room.member`s in the room state at the current forward extremities to
-        determine which users are in the room.
-
-        Will return inaccurate results for rooms with partial state, since the state for
-        the forward extremities of those rooms will exclude most members. We may also
-        calculate room state incorrectly for such rooms and believe that a member is or
-        is not in the room when the opposite is true.
+        Uses `m.room.member`s in the room state at the current forward
+        extremities to determine which users are in the room.
+
+        Will return inaccurate results for rooms with partial state, since the
+        state for the forward extremities of those rooms will exclude most
+        members. We may also calculate room state incorrectly for such rooms and
+        believe that a member is or is not in the room when the opposite is
+        true.
+
+        Note: If you only care about users in the room local to the homeserver,
+        use `get_local_users_in_room(...)` instead which will be more
+        performant.
         """
         return await self.db_pool.runInteraction(
             "get_users_in_room", self.get_users_in_room_txn, room_id

From 806b25512ebb6f80d80b5b9ebfe68749b40f09b4 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Thu, 29 Sep 2022 18:49:09 -0500
Subject: [PATCH 02/16] Remove non-appservice usages split out to other PRs

---
 synapse/handlers/room_member.py    | 2 +-
 synapse/handlers/user_directory.py | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 96cc6eae71be..88158822e009 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -1150,7 +1150,7 @@ async def transfer_room_state_on_room_upgrade(
         logger.info("Transferring room state from %s to %s", old_room_id, room_id)
 
         # Find all local users that were in the old room and copy over each user's state
-        users = await self.store.get_local_users_in_room(old_room_id)
+        users = await self.store.get_users_in_room(old_room_id)
         await self.copy_user_state_on_room_upgrade(old_room_id, room_id, users)
 
         # Add new room to the room directory if the old room was there
diff --git a/synapse/handlers/user_directory.py b/synapse/handlers/user_directory.py
index 19df4de43930..8c3c52e1caa6 100644
--- a/synapse/handlers/user_directory.py
+++ b/synapse/handlers/user_directory.py
@@ -392,7 +392,6 @@ async def _track_user_joined_room(self, room_id: str, user_id: str) -> None:
         if is_public:
             await self.store.add_users_in_public_rooms(room_id, (user_id,))
         else:
-            # TODO: get_local_users_in_room here
             users_in_room = await self.store.get_users_in_room(room_id)
             other_users_in_room = [
                 other

From 5f0f81591b43c6748c1c833504698d3677ee8151 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Thu, 29 Sep 2022 18:52:23 -0500
Subject: [PATCH 03/16] Add changelog

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

diff --git a/changelog.d/13958.bugfix b/changelog.d/13958.bugfix
new file mode 100644
index 000000000000..090bdf16ece3
--- /dev/null
+++ b/changelog.d/13958.bugfix
@@ -0,0 +1 @@
+Fix performance bottleneck with heavy appservice and bridged users in Synapse by checking appservice user interest against the local users instead of all users.

From a8be41bb80e6a165eccd60b7400c08361e23130f Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Thu, 29 Sep 2022 18:57:07 -0500
Subject: [PATCH 04/16] Revert back to using our own
 `_matches_user_in_member_list` thing

See https://github.com/matrix-org/synapse/pull/13958#discussion_r984074247
---
 synapse/appservice/__init__.py | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py
index 71b534b0397f..c28bfff0f408 100644
--- a/synapse/appservice/__init__.py
+++ b/synapse/appservice/__init__.py
@@ -172,11 +172,26 @@ async def _matches_user_in_member_list(
         Returns:
             True if this service would like to know about this room.
         """
-        app_service_users_in_room = await store.get_app_service_users_in_room(
-            room_id, self, on_invalidate=cache_context.invalidate
+        # We can use `get_local_users_in_room(...)` here because an application
+        # service can only act on behalf of users of the server it's on.
+        #
+        # In the future, we can consider re-using
+        # `store.get_app_service_users_in_room` which is very similar to this
+        # function but has a slightly worse performance than this because we
+        # have an early escape-hatch if we find a single user that the
+        # appservice is interested in. The juice would be worth the squeeze if
+        # `store.get_app_service_users_in_room` was used in more places besides
+        # an experimental MSC. But for now we can avoid doing more work and
+        # barely using it later.
+        member_list = await store.get_local_users_in_room(
+            room_id, on_invalidate=cache_context.invalidate
         )
 
-        return len(app_service_users_in_room) > 0
+        # check joined member events
+        for user_id in member_list:
+            if self.is_interested_in_user(user_id):
+                return True
+        return False
 
     def is_interested_in_user(
         self,

From 72985dffdbea21d0671673c4459986fb6cdce480 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Thu, 29 Sep 2022 18:59:41 -0500
Subject: [PATCH 05/16] Rename variables to 'local' to be obvious our intention

---
 synapse/appservice/__init__.py               | 4 ++--
 synapse/storage/databases/main/appservice.py | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py
index c28bfff0f408..a1d328cd2940 100644
--- a/synapse/appservice/__init__.py
+++ b/synapse/appservice/__init__.py
@@ -183,12 +183,12 @@ async def _matches_user_in_member_list(
         # `store.get_app_service_users_in_room` was used in more places besides
         # an experimental MSC. But for now we can avoid doing more work and
         # barely using it later.
-        member_list = await store.get_local_users_in_room(
+        local_user_ids = await store.get_local_users_in_room(
             room_id, on_invalidate=cache_context.invalidate
         )
 
         # check joined member events
-        for user_id in member_list:
+        for user_id in local_user_ids:
             if self.is_interested_in_user(user_id):
                 return True
         return False
diff --git a/synapse/storage/databases/main/appservice.py b/synapse/storage/databases/main/appservice.py
index 26fd4fa31a85..726f2547dcc7 100644
--- a/synapse/storage/databases/main/appservice.py
+++ b/synapse/storage/databases/main/appservice.py
@@ -169,10 +169,10 @@ async def get_app_service_users_in_room(
         """
         # We can use `get_local_users_in_room(...)` here because an application
         # service can only act on behalf of users of the server it's on.
-        users_in_room = await self.get_local_users_in_room(
+        local_users_in_room = await self.get_local_users_in_room(
             room_id, on_invalidate=cache_context.invalidate
         )
-        return list(filter(app_service.is_interested_in_user, users_in_room))
+        return list(filter(app_service.is_interested_in_user, local_users_in_room))
 
 
 class ApplicationServiceStore(ApplicationServiceWorkerStore):

From 92b9da2f177bfb4913d99a8ab49e27d90a9d912b Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Thu, 29 Sep 2022 19:22:47 -0500
Subject: [PATCH 06/16] Fix tests

---
 tests/appservice/test_appservice.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/appservice/test_appservice.py b/tests/appservice/test_appservice.py
index 3018d3fc6f21..d4dccfc2f070 100644
--- a/tests/appservice/test_appservice.py
+++ b/tests/appservice/test_appservice.py
@@ -43,7 +43,7 @@ def setUp(self):
 
         self.store = Mock()
         self.store.get_aliases_for_room = simple_async_mock([])
-        self.store.get_users_in_room = simple_async_mock([])
+        self.store.get_local_users_in_room = simple_async_mock([])
 
     @defer.inlineCallbacks
     def test_regex_user_id_prefix_match(self):
@@ -129,7 +129,7 @@ def test_regex_alias_match(self):
         self.store.get_aliases_for_room = simple_async_mock(
             ["#irc_foobar:matrix.org", "#athing:matrix.org"]
         )
-        self.store.get_users_in_room = simple_async_mock([])
+        self.store.get_local_users_in_room = simple_async_mock([])
         self.assertTrue(
             (
                 yield defer.ensureDeferred(
@@ -184,7 +184,7 @@ def test_regex_alias_no_match(self):
         self.store.get_aliases_for_room = simple_async_mock(
             ["#xmpp_foobar:matrix.org", "#athing:matrix.org"]
         )
-        self.store.get_users_in_room = simple_async_mock([])
+        self.store.get_local_users_in_room = simple_async_mock([])
         self.assertFalse(
             (
                 yield defer.ensureDeferred(
@@ -203,7 +203,7 @@ def test_regex_multiple_matches(self):
         self.service.namespaces[ApplicationService.NS_USERS].append(_regex("@irc_.*"))
         self.event.sender = "@irc_foobar:matrix.org"
         self.store.get_aliases_for_room = simple_async_mock(["#irc_barfoo:matrix.org"])
-        self.store.get_users_in_room = simple_async_mock([])
+        self.store.get_local_users_in_room = simple_async_mock([])
         self.assertTrue(
             (
                 yield defer.ensureDeferred(
@@ -236,7 +236,7 @@ def test_interested_in_self(self):
     def test_member_list_match(self):
         self.service.namespaces[ApplicationService.NS_USERS].append(_regex("@irc_.*"))
         # Note that @irc_fo:here is the AS user.
-        self.store.get_users_in_room = simple_async_mock(
+        self.store.get_local_users_in_room = simple_async_mock(
             ["@alice:here", "@irc_fo:here", "@bob:here"]
         )
         self.store.get_aliases_for_room = simple_async_mock([])

From 5d3c6a31dd683391d239b914b2f6908780241e61 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Fri, 30 Sep 2022 12:08:48 -0500
Subject: [PATCH 07/16] Wrapping happens at 88 chars

See https://github.com/matrix-org/synapse/pull/13958#discussion_r984106210
---
 synapse/storage/databases/main/roommember.py | 22 +++++++++-----------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py
index 8a9e553397b4..5230efffb143 100644
--- a/synapse/storage/databases/main/roommember.py
+++ b/synapse/storage/databases/main/roommember.py
@@ -152,18 +152,16 @@ async def get_users_in_room(self, room_id: str) -> List[str]:
         `get_current_hosts_in_room()` and so we can re-use the cache but it's
         not horrible to have here either.
 
-        Uses `m.room.member`s in the room state at the current forward
-        extremities to determine which users are in the room.
-
-        Will return inaccurate results for rooms with partial state, since the
-        state for the forward extremities of those rooms will exclude most
-        members. We may also calculate room state incorrectly for such rooms and
-        believe that a member is or is not in the room when the opposite is
-        true.
-
-        Note: If you only care about users in the room local to the homeserver,
-        use `get_local_users_in_room(...)` instead which will be more
-        performant.
+        Uses `m.room.member`s in the room state at the current forward extremities to
+        determine which users are in the room.
+
+        Will return inaccurate results for rooms with partial state, since the state for
+        the forward extremities of those rooms will exclude most members. We may also
+        calculate room state incorrectly for such rooms and believe that a member is or
+        is not in the room when the opposite is true.
+
+        Note: If you only care about users in the room local to the homeserver, use
+        `get_local_users_in_room(...)` instead which will be more performant.
         """
         return await self.db_pool.runInteraction(
             "get_users_in_room", self.get_users_in_room_txn, room_id

From 76435c7915c3a8d8c5b7ac345573bced20343aaf Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Fri, 30 Sep 2022 13:46:25 -0500
Subject: [PATCH 08/16] Add actual homeserver tests for local/remote
 interesting to appservice users

See https://github.com/matrix-org/synapse/pull/13958#discussion_r984470972
---
 synapse/appservice/__init__.py      |  23 ++---
 tests/appservice/test_appservice.py |   8 +-
 tests/handlers/test_appservice.py   | 139 +++++++++++++++++++++++++++-
 3 files changed, 147 insertions(+), 23 deletions(-)

diff --git a/synapse/appservice/__init__.py b/synapse/appservice/__init__.py
index a1d328cd2940..357ccf109099 100644
--- a/synapse/appservice/__init__.py
+++ b/synapse/appservice/__init__.py
@@ -172,23 +172,14 @@ async def _matches_user_in_member_list(
         Returns:
             True if this service would like to know about this room.
         """
-        # We can use `get_local_users_in_room(...)` here because an application
-        # service can only act on behalf of users of the server it's on.
-        #
-        # In the future, we can consider re-using
-        # `store.get_app_service_users_in_room` which is very similar to this
-        # function but has a slightly worse performance than this because we
-        # have an early escape-hatch if we find a single user that the
-        # appservice is interested in. The juice would be worth the squeeze if
-        # `store.get_app_service_users_in_room` was used in more places besides
-        # an experimental MSC. But for now we can avoid doing more work and
-        # barely using it later.
-        local_user_ids = await store.get_local_users_in_room(
+        # We need to get all users (local and remote) as an application service can be
+        # interested in anyone.
+        member_list = await store.get_users_in_room(
             room_id, on_invalidate=cache_context.invalidate
         )
 
         # check joined member events
-        for user_id in local_user_ids:
+        for user_id in member_list:
             if self.is_interested_in_user(user_id):
                 return True
         return False
@@ -200,9 +191,9 @@ def is_interested_in_user(
         """
         Returns whether the application is interested in a given user ID.
 
-        The appservice is considered to be interested in a user if either: the
-        user ID is in the appservice's user namespace, or if the user is the
-        appservice's configured sender_localpart.
+        The appservice is considered to be interested in a user if either: the user ID
+        is in the appservice's user namespace, or if the user is the appservice's
+        configured sender_localpart. The user can be local or remote.
 
         Args:
             user_id: The ID of the user to check.
diff --git a/tests/appservice/test_appservice.py b/tests/appservice/test_appservice.py
index d4dccfc2f070..b3af272944a1 100644
--- a/tests/appservice/test_appservice.py
+++ b/tests/appservice/test_appservice.py
@@ -43,7 +43,7 @@ def setUp(self):
 
         self.store = Mock()
         self.store.get_aliases_for_room = simple_async_mock([])
-        self.store.get_local_users_in_room = simple_async_mock([])
+        self.store.get_users_in_room = simple_async_mock([])
 
     @defer.inlineCallbacks
     def test_regex_user_id_prefix_match(self):
@@ -129,7 +129,7 @@ def test_regex_alias_match(self):
         self.store.get_aliases_for_room = simple_async_mock(
             ["#irc_foobar:matrix.org", "#athing:matrix.org"]
         )
-        self.store.get_local_users_in_room = simple_async_mock([])
+        self.store.get_users_in_room = simple_async_mock([])
         self.assertTrue(
             (
                 yield defer.ensureDeferred(
@@ -184,7 +184,7 @@ def test_regex_alias_no_match(self):
         self.store.get_aliases_for_room = simple_async_mock(
             ["#xmpp_foobar:matrix.org", "#athing:matrix.org"]
         )
-        self.store.get_local_users_in_room = simple_async_mock([])
+        self.store.get_users_in_room = simple_async_mock([])
         self.assertFalse(
             (
                 yield defer.ensureDeferred(
@@ -203,7 +203,7 @@ def test_regex_multiple_matches(self):
         self.service.namespaces[ApplicationService.NS_USERS].append(_regex("@irc_.*"))
         self.event.sender = "@irc_foobar:matrix.org"
         self.store.get_aliases_for_room = simple_async_mock(["#irc_barfoo:matrix.org"])
-        self.store.get_local_users_in_room = simple_async_mock([])
+        self.store.get_users_in_room = simple_async_mock([])
         self.assertTrue(
             (
                 yield defer.ensureDeferred(
diff --git a/tests/handlers/test_appservice.py b/tests/handlers/test_appservice.py
index af24c4984da5..6a761ac8efd0 100644
--- a/tests/handlers/test_appservice.py
+++ b/tests/handlers/test_appservice.py
@@ -22,7 +22,7 @@
 
 import synapse.rest.admin
 import synapse.storage
-from synapse.api.constants import EduTypes
+from synapse.api.constants import EduTypes, EventTypes
 from synapse.appservice import (
     ApplicationService,
     TransactionOneTimeKeyCounts,
@@ -36,7 +36,7 @@
 from synapse.util.stringutils import random_string
 
 from tests import unittest
-from tests.test_utils import make_awaitable, simple_async_mock
+from tests.test_utils import event_injection, make_awaitable, simple_async_mock
 from tests.unittest import override_config
 from tests.utils import MockClock
 
@@ -386,7 +386,8 @@ class ApplicationServicesHandlerSendEventsTestCase(unittest.HomeserverTestCase):
         receipts.register_servlets,
     ]
 
-    def prepare(self, reactor, clock, hs):
+    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer):
+        self.hs = hs
         # Mock the ApplicationServiceScheduler's _TransactionController's send method so that
         # we can track any outgoing ephemeral events
         self.send_mock = simple_async_mock()
@@ -412,6 +413,138 @@ def prepare(self, reactor, clock, hs):
             "exclusive_as_user", "password", self.exclusive_as_user_device_id
         )
 
+    def _notify_interested_services(self):
+        # This is normally set in `notify_interested_services` but we need to call the
+        # internal async version so the reactor gets pushed to completion.
+        self.hs.get_application_service_handler().current_max += 1
+        self.get_success(
+            self.hs.get_application_service_handler()._notify_interested_services(
+                RoomStreamToken(
+                    None, self.hs.get_application_service_handler().current_max
+                )
+            )
+        )
+
+    def test_match_local_room_members(self):
+        # Register an application service that's interested in local and remote user
+        interested_appservice = self._register_application_service(
+            namespaces={
+                ApplicationService.NS_USERS: [
+                    {
+                        "regex": "@local_as_user:test",
+                        "exclusive": True,
+                    },
+                ],
+            },
+        )
+
+        alice = self.register_user("alice", "pass")
+        alice_access_token = self.login("alice", "pass")
+        room_id = self.helper.create_room_as(room_creator=alice, tok=alice_access_token)
+
+        # Join the interesting user to the room
+        self.get_success(
+            event_injection.inject_member_event(
+                self.hs, room_id, "@local_as_user:test", "join"
+            )
+        )
+        # Kick the appservice into checking this membership event to get it out of the
+        # way
+        self._notify_interested_services()
+        # We don't care about the interesting user join event (this test is making sure
+        # the next thing works)
+        self.send_mock.reset_mock()
+
+        # Send a message from an uninteresting user
+        self.helper.send_event(
+            room_id,
+            type=EventTypes.Message,
+            content={
+                "msgtype": "m.text",
+                "body": "message from uninteresting user",
+            },
+            tok=alice_access_token,
+        )
+        # Kick the appservice into checking this new event
+        self._notify_interested_services()
+
+        self.send_mock.assert_called_once()
+        (
+            service,
+            events,
+            _ephemeral,
+            _to_device_messages,
+            _otks,
+            _fbks,
+            _device_list_summary,
+        ) = self.send_mock.call_args[0]
+
+        # Even though the message came from an uninsteresting user, it should still
+        # notify us because the interesting user is joined to the room.
+        self.assertEqual(service, interested_appservice)
+        self.assertEqual(events[0]["type"], "m.room.message")
+        self.assertEqual(events[0]["sender"], alice)
+
+    def test_match_remote_room_members(self):
+        # Register an application service that's interested in a remote user
+        interested_appservice = self._register_application_service(
+            namespaces={
+                ApplicationService.NS_USERS: [
+                    {
+                        "regex": "@interesting_user:remote",
+                        "exclusive": True,
+                    },
+                ],
+            },
+        )
+
+        alice = self.register_user("alice", "pass")
+        alice_access_token = self.login("alice", "pass")
+        room_id = self.helper.create_room_as(room_creator=alice, tok=alice_access_token)
+
+        # Join the interesting user to the room
+        self.get_success(
+            event_injection.inject_member_event(
+                self.hs, room_id, "@interesting_user:remote", "join"
+            )
+        )
+        # Kick the appservice into checking this membership event to get it out of the
+        # way
+        self._notify_interested_services()
+        # We don't care about the interesting user join event (this test is making sure
+        # the next thing works)
+        self.send_mock.reset_mock()
+
+        # Send a message from an uninteresting user
+        self.helper.send_event(
+            room_id,
+            type=EventTypes.Message,
+            content={
+                "msgtype": "m.text",
+                "body": "message from uninteresting user",
+            },
+            tok=alice_access_token,
+        )
+        # Kick the appservice into checking this new event
+        self._notify_interested_services()
+
+        self.send_mock.assert_called_once()
+        (
+            service,
+            events,
+            _ephemeral,
+            _to_device_messages,
+            _otks,
+            _fbks,
+            _device_list_summary,
+        ) = self.send_mock.call_args[0]
+
+        # Even though the message came from an uninsteresting user, it should still
+        # notify us because the interesting user is joined to the room.
+        self.assertEqual(service, interested_appservice)
+        self.assertEqual(events[0]["type"], "m.room.message")
+        self.assertEqual(events[0]["sender"], alice)
+
     def test_sending_read_receipt_batches_to_application_services(self):
         """Tests that a large batch of read receipts are sent correctly to
         interested application services.

From 4451998d38920248db5bf13e18fe6d085013cb70 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Fri, 30 Sep 2022 13:51:06 -0500
Subject: [PATCH 09/16] Clarify interested/control and lints

---
 synapse/storage/databases/main/appservice.py | 6 +++---
 tests/handlers/test_appservice.py            | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/synapse/storage/databases/main/appservice.py b/synapse/storage/databases/main/appservice.py
index 726f2547dcc7..0ec18d1baf92 100644
--- a/synapse/storage/databases/main/appservice.py
+++ b/synapse/storage/databases/main/appservice.py
@@ -158,14 +158,14 @@ async def get_app_service_users_in_room(
         cache_context: _CacheContext,
     ) -> List[str]:
         """
-        Get all users in a room that the appservice controls.
+        Get all users in a room that the appservice is interested in.
 
         Args:
             room_id: The room to check in.
-            app_service: The application service to check interest/control against
+            app_service: The application service to check interest against
 
         Returns:
-            List of user IDs that the appservice controls.
+            List of user IDs that the appservice is interested in.
         """
         # We can use `get_local_users_in_room(...)` here because an application
         # service can only act on behalf of users of the server it's on.
diff --git a/tests/handlers/test_appservice.py b/tests/handlers/test_appservice.py
index 6a761ac8efd0..038736b944cb 100644
--- a/tests/handlers/test_appservice.py
+++ b/tests/handlers/test_appservice.py
@@ -391,11 +391,11 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer):
         # Mock the ApplicationServiceScheduler's _TransactionController's send method so that
         # we can track any outgoing ephemeral events
         self.send_mock = simple_async_mock()
-        hs.get_application_service_handler().scheduler.txn_ctrl.send = self.send_mock
+        hs.get_application_service_handler().scheduler.txn_ctrl.send = self.send_mock  # type: ignore[assignment]
 
         # Mock out application services, and allow defining our own in tests
         self._services: List[ApplicationService] = []
-        self.hs.get_datastores().main.get_app_services = Mock(
+        self.hs.get_datastores().main.get_app_services = Mock(  # type: ignore[assignment]
             return_value=self._services
         )
 

From 1218f03a8fd2221c38f9fde3f86851ac316cdc42 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Fri, 30 Sep 2022 13:52:36 -0500
Subject: [PATCH 10/16] Revert mock

---
 tests/appservice/test_appservice.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/appservice/test_appservice.py b/tests/appservice/test_appservice.py
index b3af272944a1..3018d3fc6f21 100644
--- a/tests/appservice/test_appservice.py
+++ b/tests/appservice/test_appservice.py
@@ -236,7 +236,7 @@ def test_interested_in_self(self):
     def test_member_list_match(self):
         self.service.namespaces[ApplicationService.NS_USERS].append(_regex("@irc_.*"))
         # Note that @irc_fo:here is the AS user.
-        self.store.get_local_users_in_room = simple_async_mock(
+        self.store.get_users_in_room = simple_async_mock(
             ["@alice:here", "@irc_fo:here", "@bob:here"]
         )
         self.store.get_aliases_for_room = simple_async_mock([])

From 7bd38034f90ef352560c266dc3b6fb883c65c8d8 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Fri, 30 Sep 2022 13:55:02 -0500
Subject: [PATCH 11/16] Add test descriptions

---
 tests/handlers/test_appservice.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/handlers/test_appservice.py b/tests/handlers/test_appservice.py
index 038736b944cb..a142be38c5ef 100644
--- a/tests/handlers/test_appservice.py
+++ b/tests/handlers/test_appservice.py
@@ -426,6 +426,10 @@ def _notify_interested_services(self):
         )
 
     def test_match_local_room_members(self):
+        """
+        Test to make sure that a user local to the server and in the room is notified
+        when someone else in the room sends a message.
+        """
         # Register an application service that's interested in local and remote user
         interested_appservice = self._register_application_service(
             namespaces={
@@ -486,6 +490,10 @@ def test_match_local_room_members(self):
         self.assertEqual(events[0]["sender"], alice)
 
     def test_match_remote_room_members(self):
+        """
+        Test to make sure that a remote user that is in the room is notified when
+        someone else in the room sends a message.
+        """
         # Register an application service that's interested in a remote user
         interested_appservice = self._register_application_service(
             namespaces={

From d92fd2aeacd20901d37f3e59ff6c2c38acf1d613 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Fri, 30 Sep 2022 13:58:17 -0500
Subject: [PATCH 12/16] Revert other local change that we can't do

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

diff --git a/synapse/storage/databases/main/appservice.py b/synapse/storage/databases/main/appservice.py
index 0ec18d1baf92..9b33cd1dad60 100644
--- a/synapse/storage/databases/main/appservice.py
+++ b/synapse/storage/databases/main/appservice.py
@@ -167,12 +167,12 @@ async def get_app_service_users_in_room(
         Returns:
             List of user IDs that the appservice is interested in.
         """
-        # We can use `get_local_users_in_room(...)` here because an application
-        # service can only act on behalf of users of the server it's on.
-        local_users_in_room = await self.get_local_users_in_room(
+        # We need to get all users (local and remote) as an application service can be
+        # interested in anyone.
+        users_in_room = await self.get_users_in_room(
             room_id, on_invalidate=cache_context.invalidate
         )
-        return list(filter(app_service.is_interested_in_user, local_users_in_room))
+        return list(filter(app_service.is_interested_in_user, users_in_room))
 
 
 class ApplicationServiceStore(ApplicationServiceWorkerStore):

From e2d1b48f38b774d7d46cd3f22a4b98395b272e26 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Fri, 30 Sep 2022 14:06:22 -0500
Subject: [PATCH 13/16] Add changelog

---
 changelog.d/13958.bugfix | 1 -
 changelog.d/14000.misc   | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)
 delete mode 100644 changelog.d/13958.bugfix
 create mode 100644 changelog.d/14000.misc

diff --git a/changelog.d/13958.bugfix b/changelog.d/13958.bugfix
deleted file mode 100644
index 090bdf16ece3..000000000000
--- a/changelog.d/13958.bugfix
+++ /dev/null
@@ -1 +0,0 @@
-Fix performance bottleneck with heavy appservice and bridged users in Synapse by checking appservice user interest against the local users instead of all users.
diff --git a/changelog.d/14000.misc b/changelog.d/14000.misc
new file mode 100644
index 000000000000..ac178dc6f922
--- /dev/null
+++ b/changelog.d/14000.misc
@@ -0,0 +1 @@
+Add tests and clarify that an application service can be interested in local and remote users.

From 0214b5d54ee74ed0630a988be7f929de9de93a2d Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Fri, 30 Sep 2022 14:20:41 -0500
Subject: [PATCH 14/16] Parameterize same tests

---
 tests/handlers/test_appservice.py | 75 +++----------------------------
 1 file changed, 6 insertions(+), 69 deletions(-)

diff --git a/tests/handlers/test_appservice.py b/tests/handlers/test_appservice.py
index a142be38c5ef..04a511c8b17e 100644
--- a/tests/handlers/test_appservice.py
+++ b/tests/handlers/test_appservice.py
@@ -425,9 +425,10 @@ def _notify_interested_services(self):
             )
         )
 
-    def test_match_local_room_members(self):
+    @parameterized.expand(["@local_as_user:test", "@interesting_user:remote"])
+    def test_match_interesting_room_members(self, interesting_user):
         """
-        Test to make sure that a user local to the server and in the room is notified
+        Test to make sure that a interesting user (local or remote) in the room is notified
         when someone else in the room sends a message.
         """
         # Register an application service that's interested in local and remote user
@@ -435,72 +436,8 @@ def test_match_local_room_members(self):
             namespaces={
                 ApplicationService.NS_USERS: [
                     {
-                        "regex": "@local_as_user:test",
-                        "exclusive": True,
-                    },
-                ],
-            },
-        )
-
-        alice = self.register_user("alice", "pass")
-        alice_access_token = self.login("alice", "pass")
-        room_id = self.helper.create_room_as(room_creator=alice, tok=alice_access_token)
-
-        # Join the interesting user to the room
-        self.get_success(
-            event_injection.inject_member_event(
-                self.hs, room_id, "@local_as_user:test", "join"
-            )
-        )
-        # Kick the appservice into checking this membership event to get it out of the
-        # way
-        self._notify_interested_services()
-        # We don't care about the interesting user join event (this test is making sure
-        # the next thing works)
-        self.send_mock.reset_mock()
-
-        # Send a message from an uninteresting user
-        self.helper.send_event(
-            room_id,
-            type=EventTypes.Message,
-            content={
-                "msgtype": "m.text",
-                "body": "message from uninteresting user",
-            },
-            tok=alice_access_token,
-        )
-        # Kick the appservice into checking this new event
-        self._notify_interested_services()
-
-        self.send_mock.assert_called_once()
-        (
-            service,
-            events,
-            _ephemeral,
-            _to_device_messages,
-            _otks,
-            _fbks,
-            _device_list_summary,
-        ) = self.send_mock.call_args[0]
-
-        # Even though the message came from an uninsteresting user, it should still
-        # notify us because the interesting user is joined to the room.
-        self.assertEqual(service, interested_appservice)
-        self.assertEqual(events[0]["type"], "m.room.message")
-        self.assertEqual(events[0]["sender"], alice)
-
-    def test_match_remote_room_members(self):
-        """
-        Test to make sure that a remote user that is in the room is notified when
-        someone else in the room sends a message.
-        """
-        # Register an application service that's interested in a remote user
-        interested_appservice = self._register_application_service(
-            namespaces={
-                ApplicationService.NS_USERS: [
-                    {
-                        "regex": "@interesting_user:remote",
-                        "exclusive": True,
+                        "regex": interesting_user,
+                        "exclusive": False,
                     },
                 ],
             },
@@ -513,7 +450,7 @@ def test_match_remote_room_members(self):
         # Join the interesting user to the room
         self.get_success(
             event_injection.inject_member_event(
-                self.hs, room_id, "@interesting_user:remote", "join"
+                self.hs, room_id, interesting_user, "join"
             )
         )
         # Kick the appservice into checking this membership event to get it out of the

From ad6fedd5d4fcfd4f29a72243bead2cf8d1a9fd0a Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Fri, 30 Sep 2022 14:21:39 -0500
Subject: [PATCH 15/16] Fix comment

---
 tests/handlers/test_appservice.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/handlers/test_appservice.py b/tests/handlers/test_appservice.py
index 04a511c8b17e..6ae6de7861b5 100644
--- a/tests/handlers/test_appservice.py
+++ b/tests/handlers/test_appservice.py
@@ -431,7 +431,7 @@ def test_match_interesting_room_members(self, interesting_user):
         Test to make sure that a interesting user (local or remote) in the room is notified
         when someone else in the room sends a message.
         """
-        # Register an application service that's interested in local and remote user
+        # Register an application service that's interested in the `interesting_user`
         interested_appservice = self._register_application_service(
             namespaces={
                 ApplicationService.NS_USERS: [

From 8cad7023a9a32af0910d86cc16e0f396ac365fc6 Mon Sep 17 00:00:00 2001
From: Eric Eastwood <erice@element.io>
Date: Mon, 3 Oct 2022 16:25:16 -0500
Subject: [PATCH 16/16] Typo "uninteresting"

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
---
 tests/handlers/test_appservice.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/handlers/test_appservice.py b/tests/handlers/test_appservice.py
index 6ae6de7861b5..e3152b488d58 100644
--- a/tests/handlers/test_appservice.py
+++ b/tests/handlers/test_appservice.py
@@ -484,7 +484,7 @@ def test_match_interesting_room_members(self, interesting_user):
             _device_list_summary,
         ) = self.send_mock.call_args[0]
 
-        # Even though the message came from an uninsteresting user, it should still
+        # Even though the message came from an uninteresting user, it should still
         # notify us because the interesting user is joined to the room.
         self.assertEqual(service, interested_appservice)
         self.assertEqual(events[0]["type"], "m.room.message")