Bust `_membership_stream_cache` cache when current state changes #17732

MadLittleMods · 2024-09-18T22:28:12Z

Bust _membership_stream_cache cache when current state changes. This is particularly a problem in a state reset scenario where the membership might change without a corresponding event.

This PR is targeting a scenario where a state reset happens which causes room membership to change. Previously, the cache would just hold onto stale data and now we properly bust the cache in this scenario.

We have a few tests for these scenarios which you can see are now fixed because we can remove the FIXME where we were previously manually busting the cache in the test itself.

This is a general Synapse thing so by it's nature it helps out Sliding Sync.

Fix #17368

Prerequisite for #17929

Match when are busting _curr_state_delta_stream_cache

Dev notes

self._membership_stream_cache.entity_has_changed(state_key, stream_ordering)  # type: ignore[attr-defined]

self._membership_stream_cache.all_entities_changed(token)  # type: ignore[attr-defined]

get_sliding_sync_rooms_for_user

_invalidate_state_caches_and_stream
_invalidate_state_caches

process_replication_rows
_process_event_stream_row

_account_data_stream_cache
_device_inbox_stream_cache
_device_federation_outbox_stream_cache
_user_signature_stream_cache
presence_stream_cache

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Code style is correct
(run the linters)

Fix #17368

synapse/util/caches/stream_change_cache.py

MadLittleMods · 2024-09-18T22:37:37Z

synapse/storage/databases/main/cache.py

@@ -219,6 +219,8 @@ def process_replication_rows(
                    room_id = row.keys[0]
                    members_changed = set(row.keys[1:])
                    self._invalidate_state_caches(room_id, members_changed)
+                    for user_id in members_changed:
+                        self._membership_stream_cache.entity_has_changed(user_id, token)  # type: ignore[attr-defined]


Kinda weird to just stick this here (same with the others in process_replication_rows). Better way to organize this?

synapse/storage/databases/main/cache.py

…am_cache

MadLittleMods · 2024-10-09T23:47:02Z

synapse/storage/databases/main/events.py

+            stream_id: This is expected to be the minimum `stream_ordering` for the
+                batch of events that we are persisting; which means we do not end up in a
+                situation where workers see events before the `current_state_delta` updates.
+                FIXME: However, this function also gets called with next upcoming
+                `stream_ordering` when we re-sync the state of a partial stated room (see
+                `update_current_state(...)`) which may be "correct" but it would be good to
+                nail down what exactly is the expected value here.


Previous conversation: #17512 (comment)

I decided to define it in some way given we're using it for cache busting below and was curious if it is actually correct. Still not confident whether it's perfect for cache busting but might be good enough.

MadLittleMods · 2024-10-09T23:48:19Z

synapse/storage/databases/main/events.py

+        for user_id in members_to_cache_bust:
+            txn.call_after(
+                self.store._membership_stream_cache.entity_has_changed,
+                user_id,
+                stream_id,
+            )


This matches what we do for _curr_state_delta_stream_cache just above this

MadLittleMods · 2024-10-09T23:49:28Z

synapse/storage/databases/main/events.py

+        for user_id in members_to_cache_bust:
+            txn.call_after(
+                self.store._membership_stream_cache.entity_has_changed,
+                user_id,
+                stream_id,
+            )


This is the actual call that busts the the membership cache for the tests. I assume that is because this is what busts in monolith mode vs the other calls I've added are more for workers over replication

MadLittleMods · 2024-10-09T23:55:39Z

synapse/storage/databases/main/cache.py

+                    self._curr_state_delta_stream_cache.entity_has_changed(  # type: ignore[attr-defined]
+                        room_id, token
+                    )
+                    for user_id in members_changed:
+                        self._membership_stream_cache.entity_has_changed(user_id, token)  # type: ignore[attr-defined]


Wherever we are busting _curr_state_delta_stream_cache, we should also be busting _membership_stream_cache (at-least in the general area, expand the hidden diff to find if not visible)

We've forgotten to bust _curr_state_delta_stream_cache in various places which is why it's added and sometimes not.

…am_cache

changelog.d/17732.bugfix

…am_cache

erikjohnston

Sorry for this taking so long.

Not suggesting we do this now, but really we need to go and clean all of this up as there are two many near-identical code paths here that make it very easy to mess these things up.

erikjohnston · 2024-12-18T11:15:22Z

synapse/storage/databases/main/cache.py

+                    self._curr_state_delta_stream_cache.entity_has_changed(  # type: ignore[attr-defined]
+                        room_id, token
+                    )
+                    self._membership_stream_cache.all_entities_changed(token)  # type: ignore[attr-defined]


We don't actually insert anything into current_state_delta_stream during room deletion, so these are basically no-ops (as reading from the DB won't return anything new).

Given how frequently rooms are deleted nowadays (as we delete rooms where everyone has left after N days, at least on matrix.org) I'm minded to remove these and leave a note. Otherwise I'm worried how this will affect perf

Using the same logic, why do we call self._invalidate_caches_for_room(room_id) at all (just above)?

Perhaps we consider the caches which only affect the room_id to be fine but we also have plenty that clear all of the keys in there (None) which seem just as bad.

Using the same logic, why do we call self._invalidate_caches_for_room(room_id) at all (just above)?

We do delete a lot of stuff when deleting a room (obviously), and so those caches do contain different data than what is in the DB. So invalidation makes sense in that case?

For the current_state_delta_stream nothing actually gets inserted and so the _curr_state_delta_stream_cache cache wouldn't contain a different answer than what is in the DB

So you're saying that we don't delete from current_state_delta_stream when deleting a room. And we don't use _curr_state_delta_stream_cache for anything except when fetching current_state_delta_stream so there is no need to bust the cache at the moment in this scenario.

That looks correct:

synapse/synapse/storage/databases/main/purge_events.py

Lines 502 to 508 in 2d23250

# Other tables that we might want to consider clearing out include:

#

# - event_reports

# Given that these are intended for abuse management my initial

# inclination is to leave them in place.

#

# - current_state_delta_stream

But _membership_stream_cache is used for current_state_delta_stream and room_memberships and we do purge from room_memberships when deleting a room so it feels like we need to keep this one around.

synapse/synapse/storage/databases/main/purge_events.py

Line 461 in 2d23250

"room_memberships",

Overall, it feels very brittle to comment out _curr_state_delta_stream_cache here because the guarantee we're assuming is bound to change in the future. I can update if you insist.

And given we're only busting the _curr_state_delta_stream_cache for that specific room_id, it doesn't feel disruptive in that case.

The issue is that self._membership_stream_cache.all_entities_changed(token) will essentially clear that cache. Given we do now purge rooms frequently on matrix.org I worry it will have a noticeable impact on the cache hit ratio. Ideally we'd make it so that we didn't have to clear the entire cache, but that starts getting fiddly quickly. Hence trying to figure out if we really need to clear the cache.

But _membership_stream_cache is used for current_state_delta_stream and room_memberships and we do purge from room_memberships when deleting a room so it feels like we need to keep this one around.

Note that the stream change caches effectively only cache the absence of changes, i.e. "nothing has changed between tokens X and Y and so don't query the database". Hence, I think, if we remove rows from the DB we don't need to clear the cache as at worst it will report that something has changed at token X, at which point it will query the DB and discover nothing is there?

I've commented out self._membership_stream_cache.all_entities_changed(token) when we delete a room and added a comment with context on why we think this is safe.

Since the self._curr_state_delta_stream_cache.entity_has_changed(room_id, token) call only invalidates the specific room being deleted, I've left be. I don't think it has the same overarching impact that the all_entities_changed variants do.

synapse/util/caches/stream_change_cache.py

…am_cache

See #17732 (comment)

MadLittleMods · 2024-12-19T01:11:44Z

synapse/storage/databases/main/cache.py

                    self._invalidate_caches_for_room_events(room_id)
                    self._invalidate_caches_for_room(room_id)


Separate from this PR and in line with the fact that these code paths need to be cleaned up, there is an obvious duplication here because we call self._invalidate_caches_for_room_events(room_id) inside self._invalidate_caches_for_room(room_id) as well.

…am_cache

See #17732 (comment)

erikjohnston

Thanks! And sorry this took so long

MadLittleMods · 2025-01-08T16:11:55Z

Thanks for the review and taking the time to understand the intricacies here @erikjohnston 🦚

Bust _membership_stream_cache cache when current state changes

2bd0e63

Fix #17368

MadLittleMods added the A-Sync label Sep 18, 2024

MadLittleMods commented Sep 18, 2024

View reviewed changes

synapse/util/caches/stream_change_cache.py Show resolved Hide resolved

Add changelog

d327c62

MadLittleMods commented Sep 18, 2024

View reviewed changes

MadLittleMods marked this pull request as ready for review September 18, 2024 23:12

MadLittleMods requested a review from a team as a code owner September 18, 2024 23:12

MadLittleMods mentioned this pull request Sep 18, 2024

Sliding Sync: Avoid fetching left rooms and add back newly_left rooms #17725

Merged

5 tasks

MadLittleMods marked this pull request as draft September 18, 2024 23:29

MadLittleMods commented Sep 18, 2024

View reviewed changes

synapse/storage/databases/main/cache.py Show resolved Hide resolved

MadLittleMods added 2 commits October 9, 2024 17:26

Merge branch 'develop' into madlittlemods/17368-bust-_membership_stre…

64eb71f

…am_cache

Actually bust membership cache on monolith when current state changes

dd87620

MadLittleMods commented Oct 9, 2024

View reviewed changes

MadLittleMods mentioned this pull request Oct 9, 2024

Sliding Sync: Pre-populate room data for quick filtering/sorting #17512

Merged

17 tasks

MadLittleMods commented Oct 9, 2024

View reviewed changes

MadLittleMods added 2 commits October 9, 2024 18:50

Remove debug logs

7c53716

Fix lints

bceb4e0

MadLittleMods commented Oct 9, 2024

View reviewed changes

MadLittleMods marked this pull request as ready for review October 10, 2024 00:13

MadLittleMods added 3 commits November 12, 2024 14:30

Merge branch 'develop' into madlittlemods/17368-bust-_membership_stre…

944d1e0

…am_cache

Merge branch 'develop' into madlittlemods/17368-bust-_membership_stre…

20ff239

…am_cache

Merge branch 'develop' into madlittlemods/17368-bust-_membership_stre…

f40e649

…am_cache

MadLittleMods commented Dec 2, 2024

View reviewed changes

changelog.d/17732.bugfix Show resolved Hide resolved

MadLittleMods mentioned this pull request Dec 2, 2024

Sliding Sync: Include invite, ban, kick, targets when $LAZY-loading room members #17947

Merged

3 tasks

Merge branch 'develop' into madlittlemods/17368-bust-_membership_stre…

f5f0e36

…am_cache

MadLittleMods mentioned this pull request Dec 2, 2024

Sliding Sync: when using $LAZY always send down all members for non-gappy syncs #17929

Open

erikjohnston reviewed Dec 18, 2024

View reviewed changes

MadLittleMods added 2 commits December 18, 2024 16:14

Merge branch 'develop' into madlittlemods/17368-bust-_membership_stre…

7e328d7

…am_cache

Better all_entities_changed(stream_pos) implementation

ac06ddf

See #17732 (comment)

MadLittleMods commented Dec 19, 2024

View reviewed changes

Update test

e52f5f0

MadLittleMods requested review from erikjohnston and removed request for erikjohnston December 19, 2024 01:39

MadLittleMods added 4 commits January 6, 2025 11:13

Merge branch 'develop' into madlittlemods/17368-bust-_membership_stre…

bf695e6

…am_cache

Start of comment

2d20b71

More comment

484d045

See #17732 (comment)

Give more specific reasons on why safe

6d29960

MadLittleMods requested review from erikjohnston and removed request for erikjohnston January 6, 2025 20:09

erikjohnston approved these changes Jan 8, 2025

View reviewed changes

MadLittleMods merged commit aab3672 into develop Jan 8, 2025
39 checks passed

MadLittleMods deleted the madlittlemods/17368-bust-_membership_stream_cache branch January 8, 2025 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bust `_membership_stream_cache` cache when current state changes #17732

Bust `_membership_stream_cache` cache when current state changes #17732

MadLittleMods commented Sep 18, 2024 •

edited

Loading

MadLittleMods Sep 18, 2024 •

edited

Loading

MadLittleMods Oct 9, 2024 •

edited

Loading

MadLittleMods Oct 9, 2024

MadLittleMods Oct 9, 2024

MadLittleMods Oct 9, 2024

erikjohnston left a comment

erikjohnston Dec 18, 2024

MadLittleMods Dec 19, 2024 •

edited

Loading

erikjohnston Dec 19, 2024

MadLittleMods Dec 19, 2024 •

edited

Loading

erikjohnston Jan 6, 2025

MadLittleMods Jan 6, 2025

MadLittleMods Dec 19, 2024 •

edited

Loading

erikjohnston left a comment

MadLittleMods commented Jan 8, 2025

	# Other tables that we might want to consider clearing out include:
	#
	# - event_reports
	# Given that these are intended for abuse management my initial
	# inclination is to leave them in place.
	#
	# - current_state_delta_stream

		self._invalidate_caches_for_room_events(room_id)
		self._invalidate_caches_for_room(room_id)

Bust _membership_stream_cache cache when current state changes #17732

Bust _membership_stream_cache cache when current state changes #17732

Conversation

MadLittleMods commented Sep 18, 2024 • edited Loading

Dev notes

Pull Request Checklist

MadLittleMods Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

MadLittleMods Oct 9, 2024

Choose a reason for hiding this comment

MadLittleMods Oct 9, 2024

Choose a reason for hiding this comment

MadLittleMods Oct 9, 2024

Choose a reason for hiding this comment

erikjohnston left a comment

Choose a reason for hiding this comment

erikjohnston Dec 18, 2024

Choose a reason for hiding this comment

MadLittleMods Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

erikjohnston Dec 19, 2024

Choose a reason for hiding this comment

MadLittleMods Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

erikjohnston Jan 6, 2025

Choose a reason for hiding this comment

MadLittleMods Jan 6, 2025

Choose a reason for hiding this comment

MadLittleMods Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

erikjohnston left a comment

Choose a reason for hiding this comment

MadLittleMods commented Jan 8, 2025

Bust `_membership_stream_cache` cache when current state changes #17732

Bust `_membership_stream_cache` cache when current state changes #17732

MadLittleMods commented Sep 18, 2024 •

edited

Loading

MadLittleMods Sep 18, 2024 •

edited

Loading

MadLittleMods Oct 9, 2024 •

edited

Loading

MadLittleMods Dec 19, 2024 •

edited

Loading

MadLittleMods Dec 19, 2024 •

edited

Loading

MadLittleMods Dec 19, 2024 •

edited

Loading