Pass leave from remote invite rejection down Sliding Sync #18375

devonh · 2025-04-29T18:00:48Z

Dev notes

The sliding_sync_membership_snapshots and sliding_sync_joined_rooms database tables were added in #17512

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)

This handles membership changes for users that are not in current state event delta stream table (e.g. rejecting remote invites).

… to be made unique

…ot have the same identity

This reverts commit de7217f.

This reverts commit e0aad4b.

MadLittleMods

Note: I haven't reviewed the logic of get_sliding_sync_membership_changes

synapse/storage/databases/main/stream.py

MadLittleMods · 2025-04-30T21:22:04Z

synapse/storage/databases/main/stream.py

        ]

+    @trace
+    async def get_sliding_sync_membership_changes(


Since this uses the sliding_sync_membership_snapshots table, we have to gate it behind self.store.have_finished_sliding_sync_background_jobs().

It looks like we're using it in the fallback case as well right now:

✅ _compute_interested_rooms_new_tables -> _get_newly_joined_and_left_rooms -> get_sliding_sync_membership_changes

❌ _compute_interested_rooms_fallback -> get_room_membership_for_user_at_to_token -> _get_newly_joined_and_left_rooms -> get_sliding_sync_membership_changes

As one solution, per the comment in the following code, we could also just make this foreground update now as those tables originally shipped with Synapse 1.115.0rc1 (2024-09-10) (see #17512) and remove the fallback path.

synapse/synapse/handlers/sliding_sync/room_lists.py

Lines 206 to 223 in d59bbd8

if await self.store.have_finished_sliding_sync_background_jobs():

return await self._compute_interested_rooms_new_tables(

sync_config=sync_config,

previous_connection_state=previous_connection_state,

to_token=to_token,

from_token=from_token,

)

else:

# FIXME: This can be removed once we bump `SCHEMA_COMPAT_VERSION` and run the

# foreground update for

# `sliding_sync_joined_rooms`/`sliding_sync_membership_snapshots` (tracked by

# https://github.com/element-hq/synapse/issues/17623)

return await self._compute_interested_rooms_fallback(

sync_config=sync_config,

previous_connection_state=previous_connection_state,

to_token=to_token,

from_token=from_token,

)

Hmm, after looking into it a bit, that seems like a large undertaking that would block this fix unnecessarily.
Mainly - what would we do about populating the sliding_sync_joined_rooms_to_recalculate table in a foreground update and converting the 2 background jobs over to a foreground update is quite involved as they are pretty complex.

I think it may be better to split the impl & keep the old logic for the fallback case.
That means the fallback case would still have the bug, but realistically, who is still operating in the fallback case zone at this point?

I changed it to do just that for now.
It uses the old way while using the fallback case, which means the bug is still present.
But uses the new way when using the new tables.

If we aren't happy with this and want to wait until we have completed the DB transition for sliding sync then that PR will need to be created & merged before we can merge this one.

I think it may be better to split the impl & keep the old logic for the fallback case.
That means the fallback case would still have the bug, but realistically, who is still operating in the fallback case zone at this point?

Sounds reasonable to me.

We may want to consider refactoring GetRoomMembershipForUserAtToTokenTestCase/GetRoomMembershipForUserAtToTokenShardTestCase to work against compute_interested_rooms(...) (instead of get_room_membership_for_user_at_to_token(...) which only applies to the fallback path) so we have some better coverage of this logic. If you're feeling up for it, I think it could be useful to be more confident in these changes otherwise we can just add a future FIXME in both of those docstrings that we should tackle it in the future:

FIXME: We should refactor these tests to run against `compute_interested_rooms(...)` instead of just `get_room_membership_for_user_at_to_token(...)` which is only used in the fallback path (`_compute_interested_rooms_fallback(...)`). These scenarios do well to stress that logic and we shouldn't remove them just because we're removing the fallback path (tracked by https://github.com/element-hq/synapse/issues/17623).

We do still have a good set of end-to-end rest layer tests to cover us in a lot of scenarios.

I agree with the idea.

I started down that path but it looks like it'll be quite involved to do properly.
It should happen in a future PR that exclusively converts the tests over to handle the new code.

I'm not entirely comfortable merging these changes until those tests have been converted so we can ensure everything is still working as intended.
It'll take me quite a while to convert the tests due to a lack of simplified sliding sync knowledge. It's not too hard to technically change them over. But deciding what is okay to change requires intimate knowledge of the spec.

Hmm. So after converting that test, 12 of the tests fail when using the new tables (they pass when using the fallback tables).

But those same 12 tests also fail against the develop branch, so I don't think the changes here are making things any worse.

The tests have now been updated & #18399 has been merged. Let's see if these changes are still happy.

tests/rest/client/sliding_sync/test_rooms_invites.py

synapse/handlers/sliding_sync/__init__.py

tests/rest/client/sliding_sync/test_rooms_invites.py

tests/rest/client/sliding_sync/test_sliding_sync.py

synapse/storage/databases/main/stream.py

Co-authored-by: Eric Eastwood <[email protected]>

synapse/handlers/sliding_sync/room_lists.py

MadLittleMods · 2025-05-02T15:19:27Z

synapse/storage/databases/main/stream.py

        ]

+    @trace
+    async def get_sliding_sync_membership_changes(


I think it may be better to split the impl & keep the old logic for the fallback case.
That means the fallback case would still have the bug, but realistically, who is still operating in the fallback case zone at this point?

Sounds reasonable to me.

We may want to consider refactoring GetRoomMembershipForUserAtToTokenTestCase/GetRoomMembershipForUserAtToTokenShardTestCase to work against compute_interested_rooms(...) (instead of get_room_membership_for_user_at_to_token(...) which only applies to the fallback path) so we have some better coverage of this logic. If you're feeling up for it, I think it could be useful to be more confident in these changes otherwise we can just add a future FIXME in both of those docstrings that we should tackle it in the future:

FIXME: We should refactor these tests to run against `compute_interested_rooms(...)` instead of just `get_room_membership_for_user_at_to_token(...)` which is only used in the fallback path (`_compute_interested_rooms_fallback(...)`). These scenarios do well to stress that logic and we shouldn't remove them just because we're removing the fallback path (tracked by https://github.com/element-hq/synapse/issues/17623).

We do still have a good set of end-to-end rest layer tests to cover us in a lot of scenarios.

synapse/storage/databases/main/stream.py

…oms` (#18399) Spawning from #18375 (comment), This updates some sliding sync tests to use a higher level function in order to move test coverage to cover both fallback & new tables. Important when #18375 is merged. In other words, adjust tests to target `compute_interested_room(...)` (relevant to both new and fallback path) instead of the lower level `get_room_membership_for_user_at_to_token(...)` that only applies to the fallback path. ### Dev notes ``` SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync.ComputeInterestedRoomsTestCase_new ``` ``` SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.rest.client.sliding_sync ``` ``` SYNAPSE_POSTGRES=1 SYNAPSE_POSTGRES_USER=postgres SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync.ComputeInterestedRoomsTestCase_new.test_display_name_changes_leave_after_token_range ``` ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). 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. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Eric Eastwood <[email protected]>

…oms` (element-hq#18399) Spawning from element-hq#18375 (comment), This updates some sliding sync tests to use a higher level function in order to move test coverage to cover both fallback & new tables. Important when element-hq#18375 is merged. In other words, adjust tests to target `compute_interested_room(...)` (relevant to both new and fallback path) instead of the lower level `get_room_membership_for_user_at_to_token(...)` that only applies to the fallback path. ### Dev notes ``` SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync.ComputeInterestedRoomsTestCase_new ``` ``` SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.rest.client.sliding_sync ``` ``` SYNAPSE_POSTGRES=1 SYNAPSE_POSTGRES_USER=postgres SYNAPSE_TEST_LOG_LEVEL=INFO poetry run trial tests.handlers.test_sliding_sync.ComputeInterestedRoomsTestCase_new.test_display_name_changes_leave_after_token_range ``` ### Pull Request Checklist  * [x] Pull request is based on the develop branch * [x] Pull request includes a [changelog file](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#changelog). 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. * [x] [Code style](https://element-hq.github.io/synapse/latest/code_style.html) is correct (run the [linters](https://element-hq.github.io/synapse/latest/development/contributing_guide.html#run-the-linters)) --------- Co-authored-by: Eric Eastwood <[email protected]>

erikjohnston · 2025-05-14T14:42:26Z

FTR we should not be doing index creation or deletion in a delta file, and instead use the background updates helpers to do so. Otherwise we block start up, and if you forget CONCURRENTLY can block request processing

Follow on from #18375. This prevents blocking startup on creating the index, which can take a while --------- Co-authored-by: Devon Hudson <[email protected]>

erikjohnston and others added 15 commits March 28, 2025 16:27

WIP

340ae9b

This handles membership changes for users that are not in current state event delta stream table (e.g. rejecting remote invites).

Improve reject invite logic

74cd6d5

simplify query: left joins with e.event_id = NULL never match

17cdd4f

optimise query: the unioned subqueries can not overlap and don't need…

5ee188a

… to be made unique

add missing user_id filter to first half of the query

de7217f

don't use is for comparing strings returned from the DB; they may n…

9e41abd

…ot have the same identity

TODO: suspicion

af7a4af

TODO: note what indices we need

fd4a633

Revert "add missing user_id filter to first half of the query"

e0aad4b

This reverts commit de7217f.

Reapply "add missing user_id filter to first half of the query"

1519d4f

This reverts commit e0aad4b.

Get tests passing again

7ed2a12

Cleanup printing

223c87e

More printing cleanup

b8d1b5e

Readd union optimization

7640c59

Merge branch 'develop' into erikj/ss_reject_invites

9138ee0

devonh requested a review from a team as a code owner April 29, 2025 18:00

Add changelog entry

241ec9a

MadLittleMods requested changes Apr 30, 2025

View reviewed changes

devonh added 2 commits May 1, 2025 08:46

Comment newly_left room logic

79fc888

Move test & add comments

3e353b6

MadLittleMods reviewed May 1, 2025

View reviewed changes

devonh and others added 9 commits May 1, 2025 21:01

Update tests/rest/client/sliding_sync/test_sliding_sync.py

a7bc0ca

Co-authored-by: Eric Eastwood <[email protected]>

Remove leftover code

93b00c3

Make function take set

720f85e

Update synapse/storage/databases/main/stream.py

06b9a34

Co-authored-by: Eric Eastwood <[email protected]>

Add comment for why no membership means leave

db486c7

Filter out all membership non-changes

06209fc

Add better index on sliding_sync_membership_snapshots

c9a6a0b

Add docstring to new func

096bbfa

Readd old impl for fallback tables case

ed7c580

devonh requested review from MadLittleMods and erikjohnston May 1, 2025 23:23

MadLittleMods approved these changes May 2, 2025

View reviewed changes

devonh added 4 commits May 2, 2025 10:56

Add newlines to docstring

14c4e4d

Expand docstring

2c3f74f

Assert topological tokens not used

1f74e91

Add FIXME comment to update tests to run against new tables

5ad092e

devonh mentioned this pull request May 5, 2025

Convert Sliding Sync tests to use higher-level compute_interested_rooms #18399

Merged

3 tasks

Merge branch 'develop' into erikj/ss_reject_invites

b05f339

devonh and others added 2 commits May 7, 2025 15:08

Merge branch 'develop' into erikj/ss_reject_invites

e876851

Move db delta to new schema version

ea3c9fc

devonh merged commit 7c633f1 into develop May 8, 2025
39 checks passed

devonh deleted the erikj/ss_reject_invites branch May 8, 2025 14:28

erikjohnston mentioned this pull request May 14, 2025

Move index creation to background update #18439

Merged

erikjohnston added a commit that referenced this pull request May 15, 2025

Move index creation to background update (#18439)

f5ed52c

Follow on from #18375. This prevents blocking startup on creating the index, which can take a while --------- Co-authored-by: Devon Hudson <[email protected]>

SpiritCroc mentioned this pull request May 25, 2025

Rejected invites from other devices still show in roomlist SchildiChat/schildichat-android-next#64

Open

MadLittleMods added the A-Sync label Jun 10, 2025

	if await self.store.have_finished_sliding_sync_background_jobs():
	return await self._compute_interested_rooms_new_tables(
	sync_config=sync_config,
	previous_connection_state=previous_connection_state,
	to_token=to_token,
	from_token=from_token,
	)
	else:
	# FIXME: This can be removed once we bump `SCHEMA_COMPAT_VERSION` and run the
	# foreground update for
	# `sliding_sync_joined_rooms`/`sliding_sync_membership_snapshots` (tracked by
	# https://github.com/element-hq/synapse/issues/17623)
	return await self._compute_interested_rooms_fallback(
	sync_config=sync_config,
	previous_connection_state=previous_connection_state,
	to_token=to_token,
	from_token=from_token,
	)

Pass leave from remote invite rejection down Sliding Sync #18375

Pass leave from remote invite rejection down Sliding Sync #18375

Uh oh!

Conversation

devonh commented Apr 29, 2025 • edited by MadLittleMods Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dev notes

Pull Request Checklist

Uh oh!

MadLittleMods left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erikjohnston commented May 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

devonh commented Apr 29, 2025 •

edited by MadLittleMods

Loading