Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Clear the resync bit after resyncing device lists #9867

Merged
merged 3 commits into from
Apr 22, 2021
Merged

Clear the resync bit after resyncing device lists #9867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c2ad19c
Clear the resync bit after resyncing device lists
richvdh Apr 22, 2021
8fdfb52
fix logging
richvdh Apr 22, 2021
8b29f10
add missing description
richvdh Apr 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/9867.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug which could cause Synapse to get stuck in a loop of resyncing device lists.
7 changes: 7 additions & 0 deletions synapse/handlers/device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -926,6 +926,10 @@ async def user_device_resync(
else:
cached_devices = await self.store.get_cached_devices_for_user(user_id)
if cached_devices == {d["device_id"]: d for d in devices}:
logging.info(
"Skipping device list resync for %s, as our cache matches already",
user_id,
)
devices = []
ignore_devices = True

Expand All @@ -941,6 +945,9 @@ async def user_device_resync(
await self.store.update_remote_device_list_cache(
user_id, devices, stream_id
)
# mark the cache as valid, whether or not we actually processed any device
# list updates.
await self.store.mark_remote_user_device_cache_as_valid(user_id)
Comment on lines +948 to +950
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the objective here is to move the delete outside the if not ignore_devices condition. However, I'm not certain it's safe to move it outside the update_remote_device_list_cache transaction. @erikjohnston I think this is your code - thoughts welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only concern I have is if we mark the list as stale again between fetching the devices and checking against the cache? But if that is a problem I don't see how it isn't also a problem in the case where we do update the local device cache.

device_ids = [device["device_id"] for device in devices]

# Handle cross-signing keys.
Expand Down
19 changes: 9 additions & 10 deletions synapse/storage/databases/main/devices.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,15 @@ async def mark_remote_user_device_cache_as_stale(self, user_id: str) -> None:
keyvalues={"user_id": user_id},
values={},
insertion_values={"added_ts": self._clock.time_msec()},
desc="make_remote_user_device_cache_as_stale",
desc="mark_remote_user_device_cache_as_stale",
)

async def mark_remote_user_device_cache_as_valid(self, user_id: str) -> None:
# Remove the database entry that says we need to resync devices, after a resync
await self.db_pool.simple_delete(
table="device_lists_remote_resync",
keyvalues={"user_id": user_id},
desc="mark_remote_user_device_cache_as_valid",
)

async def mark_remote_user_device_list_as_unsubscribed(self, user_id: str) -> None:
Expand Down Expand Up @@ -1289,15 +1297,6 @@ def _update_remote_device_list_cache_txn(
lock=False,
)

# If we're replacing the remote user's device list cache presumably
# we've done a full resync, so we remove the entry that says we need
# to resync
self.db_pool.simple_delete_txn(
txn,
table="device_lists_remote_resync",
keyvalues={"user_id": user_id},
)

async def add_device_change_to_streams(
self, user_id: str, device_ids: Collection[str], hosts: List[str]
):
Expand Down