From 10cccac91367fc4c5fb3d46ee7527701becf7bde Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Mon, 6 Jul 2026 18:36:57 +0300 Subject: [PATCH] Ensure we bust the lazy loading cache for profile updates, when field values change --- synapse/handlers/sync.py | 45 ++++++++----- tests/handlers/test_sync.py | 126 ++++++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+), 16 deletions(-) diff --git a/synapse/handlers/sync.py b/synapse/handlers/sync.py index 5171b17579..a2ec04af3a 100644 --- a/synapse/handlers/sync.py +++ b/synapse/handlers/sync.py @@ -18,6 +18,7 @@ # [This file includes modifications made by New Vector Limited] # # +import hashlib import itertools import logging from typing import ( @@ -345,7 +346,10 @@ class SyncHandler: max_len=0, expiry_ms=LAZY_LOADED_MEMBERS_CACHE_MAX_AGE, ) - # ExpiringCache((User, Device)) -> LruCache(Other User ID + Field Name -> bool) + # ExpiringCache((User, Device)) + # -> LruCache( + # md5(Other User ID + Field Name + Field value) -> bool + # ) self.lazy_loaded_profile_fields_cache: ExpiringCache[ tuple[str, str | None], LruCache[str, bool] ] = ExpiringCache( @@ -356,12 +360,12 @@ class SyncHandler: max_len=0, expiry_ms=LAZY_LOADED_PROFILE_FIELDS_CACHE_MAX_AGE, ) - """This cache contains fields we have sent to clients as profile updates, - for a particular user + device combo. The cache entry is a combination of the - user + field name, with the value existing indicating the field has recently - been sent. The boolean value does not hold other significance. A missing - cache entry means "we have not sent this user + field name combo to the - syncing user". + """This cache contains fields and values we have sent to clients as profile + updates, for a particular user + device combo. The cache entry is a combination + of the user + field name + value, all hashed into md5, an existing value + indicating the field has recently been sent. The boolean value does not hold + other significance. A missing cache entry means "we have not sent this user + + field name + value combo to the syncing user". We don't manually remove entries from this cache, though it may be ignored in cases where the sync must send the field down to the client. @@ -1076,12 +1080,12 @@ class SyncHandler: def get_lazy_loaded_profile_fields_cache( self, cache_key: tuple[str, str | None] ) -> LruCache[str, bool]: - """This cache contains fields we have sent to clients as profile updates, - for a particular user + device combo. The cache entry is a combination of the - user + field name, with the value existing indicating the field has recently - been sent. The boolean value does not hold other significance. A missing - cache entry means "we have not sent this user + field name combo to the - syncing user". + """This cache contains fields and values we have sent to clients as profile + updates, for a particular user + device combo. The cache entry is a combination + of the user + field name + value, all hashed into md5, an existing value + indicating the field has recently been sent. The boolean value does not hold + other significance. A missing cache entry means "we have not sent this user + + field name + value combo to the syncing user". We don't manually remove entries from this cache, though it may be ignored in cases where the sync must send the field down to the client. @@ -2395,13 +2399,22 @@ class SyncHandler: sync_config.device_id, ) cache = self.get_lazy_loaded_profile_fields_cache(cache_key) - # Only send this users field if we haven't recently sent it - if cache.get(f"{other_user_id}-{field_name}") is None: + # Only send this users field if we haven't recently sent it. + # Our cache value to check against is an md5 of a string of + # user ID + field name + value, which ensures if the value + # changes, we'll miss the cache, thus sending the field update + # to the syncing user. + cache_value = hashlib.md5( + f"{other_user_id}-{field_name}-{profile_data.get(field_name)}".encode( + "utf8" + ) + ).hexdigest() + if cache.get(cache_value) is None: per_user_updates[field_name] = profile_data.get(field_name) # Update our cache to indicate this user/field combo # has been recently sent. cache.set( - f"{other_user_id}-{field_name}", + cache_value, True, ) else: diff --git a/tests/handlers/test_sync.py b/tests/handlers/test_sync.py index b972a06de0..f9062236a4 100644 --- a/tests/handlers/test_sync.py +++ b/tests/handlers/test_sync.py @@ -2332,6 +2332,132 @@ class SyncProfileUpdatesTestCase(tests.unittest.HomeserverTestCase): incremental_result.profile_updates["@third_user:test"], ) + @parameterized.expand( + [ + ["string value", "new string value"], + [True, False], + [None, "not None"], + [[], ["with item"]], + [{}, {"key": "value"}], + [{"foo": "bar"}, {"bar": "foo"}], + [42, 42.24], + ] + ) + @override_config({"include_profile_updates_in_sync": True}) + def test_profile_updates_dont_get_silenced_by_cache( + self, + value: str | bool | list | dict | int | float | None, + new_value: str | bool | list | dict | int | float | None, + ) -> None: + """Test that with MSC4429 enabled the incremental lazy sync response + includes all the profile update changes for the user, even if the profile + field has been recently sent and is in our lazy loading cache. + + Parameterize across different types of potential value types that profile + field updates could have to ensure robustness. + """ + requester = create_requester(self.user) + initial_result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, + sync_config=generate_sync_config( + user_id=self.user, + filter_collection=FilterCollection( + hs=self.hs, + filter_json={ + "org.matrix.msc4429.profile_fields": {"ids": ["field"]}, + "room": { + "state": { + "lazy_load_members": True, + }, + }, + }, + ), + ), + request_key=generate_request_key(), + ) + ) + self.assertFalse( + "@other_user:test" in initial_result.profile_updates, + ) + + # Update the field + self.get_success( + self.profile_handler.set_field( + target_user=UserID.from_string(self.other_user), + requester=create_requester(self.other_user), + field_name="field", + new_value=cast(JsonValue | dict[str, JsonValue], value), + ) + ) + incremental_result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, + since_token=initial_result.next_batch, + sync_config=generate_sync_config( + user_id=self.user, + filter_collection=FilterCollection( + hs=self.hs, + filter_json={ + "org.matrix.msc4429.profile_fields": {"ids": ["field"]}, + "room": { + "state": { + "lazy_load_members": True, + }, + }, + }, + ), + ), + request_key=generate_request_key(), + ) + ) + # We should have the field change in our sync response. + # It will also be added to the lazy loading cache, so the same field value + # isn't sent again immediately. + assert incremental_result.profile_updates["@other_user:test"] is not None + self.assertEqual( + incremental_result.profile_updates["@other_user:test"]["field"], + value, + ) + + # Update the field again, busting our cache + self.get_success( + self.profile_handler.set_field( + target_user=UserID.from_string(self.other_user), + requester=create_requester(self.other_user), + field_name="field", + new_value=cast(JsonValue | dict[str, JsonValue], new_value), + ) + ) + incremental_result = self.get_success( + self.sync_handler.wait_for_sync_for_user( + requester, + since_token=incremental_result.next_batch, + sync_config=generate_sync_config( + user_id=self.user, + filter_collection=FilterCollection( + hs=self.hs, + filter_json={ + "org.matrix.msc4429.profile_fields": {"ids": ["field"]}, + "room": { + "state": { + "lazy_load_members": True, + }, + }, + }, + ), + ), + request_key=generate_request_key(), + ) + ) + # Even though the field was added to the lazy loading members cache, + # it should come through as an update, as the field value changed. + assert incremental_result.profile_updates["@other_user:test"] is not None + self.assertEqual( + incremental_result.profile_updates["@other_user:test"]["field"], + new_value, + ) + class SyncStateAfterTestCase(tests.unittest.HomeserverTestCase): """Tests Sync Handler state behavior when using `use_state_after."""