From acc97ea8d320248db83c36f5bbbc3374b71f1f57 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Mon, 23 May 2022 17:59:16 +0100
Subject: [PATCH 01/11] Background task to delete devices not accessed for over
 a specified amount of time

---
 .../configuration/config_documentation.md     | 13 ++++++++++
 synapse/config/server.py                      | 11 +++++++++
 synapse/handlers/device.py                    | 24 +++++++++++++++++++
 synapse/storage/databases/main/devices.py     | 24 +++++++++++++++++++
 4 files changed, 72 insertions(+)

diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md
index 3ad3085bfac1..be4bda9f81f3 100644
--- a/docs/usage/configuration/config_documentation.md
+++ b/docs/usage/configuration/config_documentation.md
@@ -567,6 +567,19 @@ Example configuration:
 dummy_events_threshold: 5
 ```
 ---
+Config option `delete_stale_devices_after`
+
+An optional duration. If set, Synapse will run an hourly background task to log out and
+delete any device that hasn't been accessed for more than the specified amount of time.
+
+Defaults to no duration, which means devices aren't pruned based on the time they were
+last accessed.
+
+Example configuration:
+```yaml
+delete_stale_devices_after: 1y
+```
+
 ## Homeserver blocking ##
 Useful options for Synapse admins.
 
diff --git a/synapse/config/server.py b/synapse/config/server.py
index f73d5e1f6666..53365747fa2d 100644
--- a/synapse/config/server.py
+++ b/synapse/config/server.py
@@ -679,6 +679,17 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
             config.get("exclude_rooms_from_sync") or []
         )
 
+        delete_stale_devices_after: Optional[str] = (
+            config.get("delete_stale_devices_after") or None
+        )
+
+        if delete_stale_devices_after is not None:
+            self.delete_stale_devices_after = self.parse_duration(
+                delete_stale_devices_after
+            )
+        else:
+            self.delete_stale_devices_after = None
+
     def has_tls_listener(self) -> bool:
         return any(listener.tls for listener in self.listeners)
 
diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py
index 1d6d1f8a9248..289a3f339dff 100644
--- a/synapse/handlers/device.py
+++ b/synapse/handlers/device.py
@@ -292,6 +292,18 @@ def __init__(self, hs: "HomeServer"):
         # On start up check if there are any updates pending.
         hs.get_reactor().callWhenRunning(self._handle_new_device_update_async)
 
+        self._delete_stale_devices_after = hs.config.server.delete_stale_devices_after
+
+        if (
+            hs.config.worker.run_background_tasks
+            and self._delete_stale_devices_after is not None
+        ):
+            self.clock.looping_call(
+                run_as_background_process,
+                60 * 60 * 1000,
+                self._delete_old_devices,
+            )
+
     def _check_device_name_length(self, name: Optional[str]) -> None:
         """
         Checks whether a device name is longer than the maximum allowed length.
@@ -367,6 +379,18 @@ async def check_device_registered(
 
         raise errors.StoreError(500, "Couldn't generate a device ID.")
 
+    async def _delete_old_devices(self) -> None:
+        """Background task that deletes devices which haven't been accessed for more than
+        a configured time period.
+        """
+        now_ms = self.clock.time_msec()
+        devices = await self.store.get_devices_not_accessed_since(
+            now_ms - self._delete_stale_devices_after
+        )
+
+        for device in devices:
+            await self.delete_device(device["user_id"], device["device_id"])
+
     @trace
     async def delete_device(self, user_id: str, device_id: str) -> None:
         """Delete the given device
diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py
index 2df4dd4ed423..2722439e0bed 100644
--- a/synapse/storage/databases/main/devices.py
+++ b/synapse/storage/databases/main/devices.py
@@ -1153,6 +1153,30 @@ def _prune_txn(txn: LoggingTransaction) -> None:
             _prune_txn,
         )
 
+    async def get_devices_not_accessed_since(self, since_ms: int) -> List[Dict[str, str]]:
+        """Retrieves a list of all devices that haven't been accessed since a given date.
+
+        Args:
+            since_ms: the timestamp to select on, every device which last access dates
+                from before that time is returned.
+
+        Returns:
+            A list of device IDs.
+        """
+
+        def get_devices_not_accessed_since_txn(txn: LoggingTransaction) -> List[Dict[str, str]]:
+            sql = """
+                SELECT user_id, device_id
+                FROM devices WHERE last_seen < ? AND hidden = FALSE
+            """
+            txn.execute(sql, (since_ms,))
+            return self.db_pool.cursor_to_dict(txn)
+
+        return await self.db_pool.runInteraction(
+            "get_devices_not_accessed_since",
+            get_devices_not_accessed_since_txn,
+        )
+
 
 class DeviceBackgroundUpdateStore(SQLBaseStore):
     def __init__(

From df96732f70b47f140355c36c21b5ae9626b763f9 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 24 May 2022 14:20:22 +0100
Subject: [PATCH 02/11] Prepare tests

---
 tests/rest/client/{test_device_lists.py => test_devices.py} | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename tests/rest/client/{test_device_lists.py => test_devices.py} (100%)

diff --git a/tests/rest/client/test_device_lists.py b/tests/rest/client/test_devices.py
similarity index 100%
rename from tests/rest/client/test_device_lists.py
rename to tests/rest/client/test_devices.py

From 7f1df5ff409f8697c748abee656f358564c6bf15 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 24 May 2022 14:53:02 +0100
Subject: [PATCH 03/11] Lint and docs

---
 synapse/config/server.py                  |  2 +-
 synapse/handlers/device.py                | 18 +++++++++++-------
 synapse/storage/databases/main/devices.py | 12 ++++++++----
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/synapse/config/server.py b/synapse/config/server.py
index 53365747fa2d..657322cb1f98 100644
--- a/synapse/config/server.py
+++ b/synapse/config/server.py
@@ -684,7 +684,7 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
         )
 
         if delete_stale_devices_after is not None:
-            self.delete_stale_devices_after = self.parse_duration(
+            self.delete_stale_devices_after: Optional[int] = self.parse_duration(
                 delete_stale_devices_after
             )
         else:
diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py
index 289a3f339dff..87154b3138af 100644
--- a/synapse/handlers/device.py
+++ b/synapse/handlers/device.py
@@ -294,14 +294,15 @@ def __init__(self, hs: "HomeServer"):
 
         self._delete_stale_devices_after = hs.config.server.delete_stale_devices_after
 
-        if (
-            hs.config.worker.run_background_tasks
-            and self._delete_stale_devices_after is not None
-        ):
+        # Ideally we would run this on a worker and condition this on the
+        # "run_background_tasks_on" setting, but this would mean making the notification
+        # of device list changes over federation work on workers, which is nontrivial.
+        if self._delete_stale_devices_after is not None:
             self.clock.looping_call(
                 run_as_background_process,
                 60 * 60 * 1000,
-                self._delete_old_devices,
+                "delete_stale_devices",
+                self._delete_stale_devices,
             )
 
     def _check_device_name_length(self, name: Optional[str]) -> None:
@@ -379,10 +380,12 @@ async def check_device_registered(
 
         raise errors.StoreError(500, "Couldn't generate a device ID.")
 
-    async def _delete_old_devices(self) -> None:
+    async def _delete_stale_devices(self) -> None:
         """Background task that deletes devices which haven't been accessed for more than
         a configured time period.
         """
+        # We should only be running this job if the config option is defined.
+        assert self._delete_stale_devices_after is not None
         now_ms = self.clock.time_msec()
         devices = await self.store.get_devices_not_accessed_since(
             now_ms - self._delete_stale_devices_after
@@ -713,7 +716,8 @@ async def _handle_new_device_update_async(self) -> None:
                             )
                             # TODO: when called, this isn't in a logging context.
                             # This leads to log spam, sentry event spam, and massive
-                            # memory usage. See #12552.
+                            # memory usage.
+                            # See https://github.com/matrix-org/synapse/issues/12552.
                             # log_kv(
                             #     {"message": "sent device update to host", "host": host}
                             # )
diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py
index 2722439e0bed..7c57b60498cc 100644
--- a/synapse/storage/databases/main/devices.py
+++ b/synapse/storage/databases/main/devices.py
@@ -1153,7 +1153,9 @@ def _prune_txn(txn: LoggingTransaction) -> None:
             _prune_txn,
         )
 
-    async def get_devices_not_accessed_since(self, since_ms: int) -> List[Dict[str, str]]:
+    async def get_devices_not_accessed_since(
+        self, since_ms: int
+    ) -> List[Dict[str, str]]:
         """Retrieves a list of all devices that haven't been accessed since a given date.
 
         Args:
@@ -1161,10 +1163,13 @@ async def get_devices_not_accessed_since(self, since_ms: int) -> List[Dict[str,
                 from before that time is returned.
 
         Returns:
-            A list of device IDs.
+            A list of dictionary, each indicating the user ID and device ID of a device
+            that hasn't been accessed since the given date.
         """
 
-        def get_devices_not_accessed_since_txn(txn: LoggingTransaction) -> List[Dict[str, str]]:
+        def get_devices_not_accessed_since_txn(
+            txn: LoggingTransaction,
+        ) -> List[Dict[str, str]]:
             sql = """
                 SELECT user_id, device_id
                 FROM devices WHERE last_seen < ? AND hidden = FALSE
@@ -1177,7 +1182,6 @@ def get_devices_not_accessed_since_txn(txn: LoggingTransaction) -> List[Dict[str
             get_devices_not_accessed_since_txn,
         )
 
-
 class DeviceBackgroundUpdateStore(SQLBaseStore):
     def __init__(
         self,

From 60bcababf3a696145749c4d7c25b439683c19810 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 24 May 2022 14:53:16 +0100
Subject: [PATCH 04/11] Add a test case

---
 tests/rest/client/test_devices.py | 42 +++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/tests/rest/client/test_devices.py b/tests/rest/client/test_devices.py
index a8af4e2435ac..03a98f61f9ae 100644
--- a/tests/rest/client/test_devices.py
+++ b/tests/rest/client/test_devices.py
@@ -13,8 +13,13 @@
 # limitations under the License.
 from http import HTTPStatus
 
+from twisted.internet.testing import MemoryReactor
+
+from synapse.api.errors import NotFoundError
 from synapse.rest import admin, devices, room, sync
 from synapse.rest.client import account, login, register
+from synapse.server import HomeServer
+from synapse.util import Clock
 
 from tests import unittest
 
@@ -157,3 +162,40 @@ def test_not_receiving_local_device_list_changes(self) -> None:
         self.assertNotIn(
             alice_user_id, changed_device_lists, bob_sync_channel.json_body
         )
+
+
+class DevicesTestCase(unittest.HomeserverTestCase):
+    servlets = [
+        admin.register_servlets,
+        login.register_servlets,
+        sync.register_servlets,
+    ]
+
+    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer):
+        self.handler = hs.get_device_handler()
+
+    @unittest.override_config({"delete_stale_devices_after": 3600000})
+    def test_delete_stale_devices(self) -> None:
+        """Tests that stale devices are automatically removed after a set time of
+        inactivity.
+        """
+        # Register a user and creates 2 devices for them.
+        user_id = self.register_user("user", "password")
+        tok1 = self.login("user", "password", device_id="abc")
+        tok2 = self.login("user", "password", device_id="def")
+
+        # Sync them so they have a last_seen value.
+        self.make_request("GET", "/sync", access_token=tok1)
+        self.make_request("GET", "/sync", access_token=tok2)
+
+        # Advance half an hour and sync again with one of the devices, so that the next
+        # time the background job runs we don't delete this device (since it will look
+        # for devices that haven't been used for over an hour).
+        self.reactor.advance(1800)
+        self.make_request("GET", "/sync", access_token=tok1)
+
+        # Advance another half an hour, and check that the device that has synced still
+        # exists but the one that hasn't has been removed.
+        self.reactor.advance(1800)
+        self.get_success(self.handler.get_device(user_id, "abc"))
+        self.get_failure(self.handler.get_device(user_id, "def"), NotFoundError)

From c9f474cc7d76d0ee257a584277e7e574124e2d8d Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 24 May 2022 15:03:16 +0100
Subject: [PATCH 05/11] Changelog

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

diff --git a/changelog.d/12855.feature b/changelog.d/12855.feature
new file mode 100644
index 000000000000..915f008ec673
--- /dev/null
+++ b/changelog.d/12855.feature
@@ -0,0 +1 @@
+Add a configurable background job to delete stale devices.

From 9735b37eff34d03a7723c5bbedb4c151dcdfe169 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Tue, 24 May 2022 15:06:17 +0100
Subject: [PATCH 06/11] Lint

---
 synapse/storage/databases/main/devices.py | 1 +
 tests/rest/client/test_devices.py         | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py
index 7c57b60498cc..fefb1eea948b 100644
--- a/synapse/storage/databases/main/devices.py
+++ b/synapse/storage/databases/main/devices.py
@@ -1182,6 +1182,7 @@ def get_devices_not_accessed_since_txn(
             get_devices_not_accessed_since_txn,
         )
 
+
 class DeviceBackgroundUpdateStore(SQLBaseStore):
     def __init__(
         self,
diff --git a/tests/rest/client/test_devices.py b/tests/rest/client/test_devices.py
index 03a98f61f9ae..71495d04f9a4 100644
--- a/tests/rest/client/test_devices.py
+++ b/tests/rest/client/test_devices.py
@@ -171,7 +171,7 @@ class DevicesTestCase(unittest.HomeserverTestCase):
         sync.register_servlets,
     ]
 
-    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer):
+    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
         self.handler = hs.get_device_handler()
 
     @unittest.override_config({"delete_stale_devices_after": 3600000})

From 4aa04e833a80907efa80201f523fbdf0cae385c4 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 25 May 2022 11:36:00 +0200
Subject: [PATCH 07/11] Apply suggestions from code review

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 docs/usage/configuration/config_documentation.md | 3 +--
 synapse/storage/databases/main/devices.py        | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md
index be4bda9f81f3..f974b155e861 100644
--- a/docs/usage/configuration/config_documentation.md
+++ b/docs/usage/configuration/config_documentation.md
@@ -572,8 +572,7 @@ Config option `delete_stale_devices_after`
 An optional duration. If set, Synapse will run an hourly background task to log out and
 delete any device that hasn't been accessed for more than the specified amount of time.
 
-Defaults to no duration, which means devices aren't pruned based on the time they were
-last accessed.
+Defaults to no duration, which means devices are never pruned.
 
 Example configuration:
 ```yaml
diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py
index fefb1eea948b..302c96de5757 100644
--- a/synapse/storage/databases/main/devices.py
+++ b/synapse/storage/databases/main/devices.py
@@ -1159,7 +1159,7 @@ async def get_devices_not_accessed_since(
         """Retrieves a list of all devices that haven't been accessed since a given date.
 
         Args:
-            since_ms: the timestamp to select on, every device which last access dates
+            since_ms: the timestamp to select on, every device with a last access date
                 from before that time is returned.
 
         Returns:

From b61a29d6b954beedf6b6b0b47ff3746f4259b94a Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Wed, 25 May 2022 12:06:34 +0100
Subject: [PATCH 08/11] Incorporate review

---
 .../usage/configuration/config_documentation.md |  2 +-
 synapse/handlers/device.py                      | 17 ++++++++++-------
 synapse/storage/databases/main/devices.py       | 17 +++++++++++++----
 tests/rest/client/test_devices.py               | 11 ++++++-----
 4 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/docs/usage/configuration/config_documentation.md b/docs/usage/configuration/config_documentation.md
index f974b155e861..2184c3a48cdd 100644
--- a/docs/usage/configuration/config_documentation.md
+++ b/docs/usage/configuration/config_documentation.md
@@ -569,7 +569,7 @@ dummy_events_threshold: 5
 ---
 Config option `delete_stale_devices_after`
 
-An optional duration. If set, Synapse will run an hourly background task to log out and
+An optional duration. If set, Synapse will run a daily background task to log out and
 delete any device that hasn't been accessed for more than the specified amount of time.
 
 Defaults to no duration, which means devices are never pruned.
diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py
index 87154b3138af..ecaf7bbd1fd3 100644
--- a/synapse/handlers/device.py
+++ b/synapse/handlers/device.py
@@ -61,6 +61,7 @@
 logger = logging.getLogger(__name__)
 
 MAX_DEVICE_DISPLAY_NAME_LEN = 100
+DELETE_STALE_DEVICES_INTERVAL_MS = 24 * 60 * 60 * 1000
 
 
 class DeviceWorkerHandler:
@@ -300,7 +301,7 @@ def __init__(self, hs: "HomeServer"):
         if self._delete_stale_devices_after is not None:
             self.clock.looping_call(
                 run_as_background_process,
-                60 * 60 * 1000,
+                DELETE_STALE_DEVICES_INTERVAL_MS,
                 "delete_stale_devices",
                 self._delete_stale_devices,
             )
@@ -387,12 +388,14 @@ async def _delete_stale_devices(self) -> None:
         # We should only be running this job if the config option is defined.
         assert self._delete_stale_devices_after is not None
         now_ms = self.clock.time_msec()
-        devices = await self.store.get_devices_not_accessed_since(
-            now_ms - self._delete_stale_devices_after
-        )
-
-        for device in devices:
-            await self.delete_device(device["user_id"], device["device_id"])
+        since_ms = now_ms - self._delete_stale_devices_after
+        devices = await self.store.get_devices_not_accessed_since(since_ms)
+
+        for user_id, user_devices in devices.items():
+            # The devices table can contain devices for remote users, and we don't want
+            # e.g. break encryption by accidentally removing them.
+            if self.hs.is_mine_id(user_id):
+                await self.delete_devices(user_id, user_devices)
 
     @trace
     async def delete_device(self, user_id: str, device_id: str) -> None:
diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py
index 302c96de5757..41bd4492af46 100644
--- a/synapse/storage/databases/main/devices.py
+++ b/synapse/storage/databases/main/devices.py
@@ -1155,7 +1155,7 @@ def _prune_txn(txn: LoggingTransaction) -> None:
 
     async def get_devices_not_accessed_since(
         self, since_ms: int
-    ) -> List[Dict[str, str]]:
+    ) -> Dict[str, List[str]]:
         """Retrieves a list of all devices that haven't been accessed since a given date.
 
         Args:
@@ -1163,8 +1163,9 @@ async def get_devices_not_accessed_since(
                 from before that time is returned.
 
         Returns:
-            A list of dictionary, each indicating the user ID and device ID of a device
-            that hasn't been accessed since the given date.
+            A dictionary with an entry for each user with at least one device matching
+            the request, which value is a list of the device ID(s) for the corresponding
+            device(s).
         """
 
         def get_devices_not_accessed_since_txn(
@@ -1177,11 +1178,19 @@ def get_devices_not_accessed_since_txn(
             txn.execute(sql, (since_ms,))
             return self.db_pool.cursor_to_dict(txn)
 
-        return await self.db_pool.runInteraction(
+        rows = await self.db_pool.runInteraction(
             "get_devices_not_accessed_since",
             get_devices_not_accessed_since_txn,
         )
 
+        devices: Dict[str, List[str]] = {}
+        for row in rows:
+            user_devices = devices.get(row["user_id"], [])
+            user_devices.append(row["device_id"])
+            devices[row["user_id"]] = user_devices
+
+        return devices
+
 
 class DeviceBackgroundUpdateStore(SQLBaseStore):
     def __init__(
diff --git a/tests/rest/client/test_devices.py b/tests/rest/client/test_devices.py
index 71495d04f9a4..7565f54fb24f 100644
--- a/tests/rest/client/test_devices.py
+++ b/tests/rest/client/test_devices.py
@@ -174,10 +174,11 @@ class DevicesTestCase(unittest.HomeserverTestCase):
     def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
         self.handler = hs.get_device_handler()
 
-    @unittest.override_config({"delete_stale_devices_after": 3600000})
+    @unittest.override_config({"delete_stale_devices_after": 72000000})
     def test_delete_stale_devices(self) -> None:
         """Tests that stale devices are automatically removed after a set time of
         inactivity.
+        The configuration is set to delete devices that haven't been used in the past 20h.
         """
         # Register a user and creates 2 devices for them.
         user_id = self.register_user("user", "password")
@@ -188,14 +189,14 @@ def test_delete_stale_devices(self) -> None:
         self.make_request("GET", "/sync", access_token=tok1)
         self.make_request("GET", "/sync", access_token=tok2)
 
-        # Advance half an hour and sync again with one of the devices, so that the next
+        # Advance half a day and sync again with one of the devices, so that the next
         # time the background job runs we don't delete this device (since it will look
         # for devices that haven't been used for over an hour).
-        self.reactor.advance(1800)
+        self.reactor.advance(43200)
         self.make_request("GET", "/sync", access_token=tok1)
 
-        # Advance another half an hour, and check that the device that has synced still
+        # Advance another half a day, and check that the device that has synced still
         # exists but the one that hasn't has been removed.
-        self.reactor.advance(1800)
+        self.reactor.advance(43200)
         self.get_success(self.handler.get_device(user_id, "abc"))
         self.get_failure(self.handler.get_device(user_id, "def"), NotFoundError)

From eee1864c8d481ee1f5f539da9bb5cff02cd71363 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Fri, 27 May 2022 15:16:19 +0200
Subject: [PATCH 09/11] Apply suggestions from code review

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 synapse/storage/databases/main/devices.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py
index 41bd4492af46..46c1196acca2 100644
--- a/synapse/storage/databases/main/devices.py
+++ b/synapse/storage/databases/main/devices.py
@@ -1156,7 +1156,7 @@ def _prune_txn(txn: LoggingTransaction) -> None:
     async def get_devices_not_accessed_since(
         self, since_ms: int
     ) -> Dict[str, List[str]]:
-        """Retrieves a list of all devices that haven't been accessed since a given date.
+        """Retrieves devices that haven't been accessed since a given date.
 
         Args:
             since_ms: the timestamp to select on, every device with a last access date
@@ -1185,9 +1185,8 @@ def get_devices_not_accessed_since_txn(
 
         devices: Dict[str, List[str]] = {}
         for row in rows:
-            user_devices = devices.get(row["user_id"], [])
+            user_devices = devices.setdefault(row["user_id"], [])
             user_devices.append(row["device_id"])
-            devices[row["user_id"]] = user_devices
 
         return devices
 

From 06f027a36ba29af9ee34fa6befaa70872cbbb786 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Fri, 27 May 2022 15:55:11 +0100
Subject: [PATCH 10/11] Only retrieve local devices

---
 synapse/handlers/device.py                |  7 ++-----
 synapse/storage/databases/main/devices.py | 10 ++++++----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/synapse/handlers/device.py b/synapse/handlers/device.py
index ecaf7bbd1fd3..83a3e10ea021 100644
--- a/synapse/handlers/device.py
+++ b/synapse/handlers/device.py
@@ -389,13 +389,10 @@ async def _delete_stale_devices(self) -> None:
         assert self._delete_stale_devices_after is not None
         now_ms = self.clock.time_msec()
         since_ms = now_ms - self._delete_stale_devices_after
-        devices = await self.store.get_devices_not_accessed_since(since_ms)
+        devices = await self.store.get_local_devices_not_accessed_since(since_ms)
 
         for user_id, user_devices in devices.items():
-            # The devices table can contain devices for remote users, and we don't want
-            # e.g. break encryption by accidentally removing them.
-            if self.hs.is_mine_id(user_id):
-                await self.delete_devices(user_id, user_devices)
+            await self.delete_devices(user_id, user_devices)
 
     @trace
     async def delete_device(self, user_id: str, device_id: str) -> None:
diff --git a/synapse/storage/databases/main/devices.py b/synapse/storage/databases/main/devices.py
index 46c1196acca2..2e31109ff275 100644
--- a/synapse/storage/databases/main/devices.py
+++ b/synapse/storage/databases/main/devices.py
@@ -1153,10 +1153,10 @@ def _prune_txn(txn: LoggingTransaction) -> None:
             _prune_txn,
         )
 
-    async def get_devices_not_accessed_since(
+    async def get_local_devices_not_accessed_since(
         self, since_ms: int
     ) -> Dict[str, List[str]]:
-        """Retrieves devices that haven't been accessed since a given date.
+        """Retrieves local devices that haven't been accessed since a given date.
 
         Args:
             since_ms: the timestamp to select on, every device with a last access date
@@ -1185,8 +1185,10 @@ def get_devices_not_accessed_since_txn(
 
         devices: Dict[str, List[str]] = {}
         for row in rows:
-            user_devices = devices.setdefault(row["user_id"], [])
-            user_devices.append(row["device_id"])
+            # Remote devices are never stale from our point of view.
+            if self.hs.is_mine_id(row["user_id"]):
+                user_devices = devices.setdefault(row["user_id"], [])
+                user_devices.append(row["device_id"])
 
         return devices
 

From ca188e17449a9c75573f1e5ac465d9c31aa7b831 Mon Sep 17 00:00:00 2001
From: Brendan Abolivier <babolivier@matrix.org>
Date: Fri, 27 May 2022 16:23:09 +0100
Subject: [PATCH 11/11] Fix import for reactor class

---
 tests/rest/client/test_devices.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/rest/client/test_devices.py b/tests/rest/client/test_devices.py
index 7565f54fb24f..aa98222434ab 100644
--- a/tests/rest/client/test_devices.py
+++ b/tests/rest/client/test_devices.py
@@ -13,7 +13,7 @@
 # limitations under the License.
 from http import HTTPStatus
 
-from twisted.internet.testing import MemoryReactor
+from twisted.test.proto_helpers import MemoryReactor
 
 from synapse.api.errors import NotFoundError
 from synapse.rest import admin, devices, room, sync