Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix newly_left rooms not appearing if we returned early (Sliding Sync) #17301

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Fix newly_left rooms not appearing if we returned early (Sliding Sync) #17301

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
acef476
Fix `newly_left` rooms not appearing if we returned early
MadLittleMods Jun 12, 2024
378a73b
Add changelog
MadLittleMods Jun 12, 2024
95aa248
Merge branch 'develop' into madlittlemods/fix-newly-left-rooms-slidin…
MadLittleMods Jun 13, 2024
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/17301.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add initial implementation of an experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint.
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Matches the changelog in #17187 so they merge since that hasn't shipped in a release yet AFAIK.

Copy link
Member

Choose a reason for hiding this comment

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

Heads up that merging entries only works if the two newsfiles have the same extension (.feature vs .bugfix wouldn't work).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I don't think I would have noticed, I've updated the docs with this detail 👍 -> #17399

26 changes: 13 additions & 13 deletions synapse/handlers/sliding_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,12 +275,6 @@ async def get_sync_room_ids_for_user(
instance_map=immutabledict(instance_to_max_stream_ordering_map),
)

# If our `to_token` is already the same or ahead of the latest room membership
# for the user, we can just straight-up return the room list (nothing has
# changed)
if membership_snapshot_token.is_before_or_eq(to_token.room_key):
return sync_room_id_set
Comment on lines -281 to -282
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could have also fixed this by making sure from_token is None in this condition:

Suggested change
if membership_snapshot_token.is_before_or_eq(to_token.room_key):
return sync_room_id_set
if from_token is None and membership_snapshot_token.is_before_or_eq(to_token.room_key):
return sync_room_id_set

But the way the PR is now, puts the condition next to the relevant "1)" fixups and we can avoid the extra empty membership lookup if we only need to do the newly_left fixup "2)"


# Since we fetched the users room list at some point in time after the from/to
# tokens, we need to revert/rewind some membership changes to match the point in
# time of the `to_token`. In particular, we need to make these fixups:
Expand All @@ -300,14 +294,20 @@ async def get_sync_room_ids_for_user(

# 1) Fetch membership changes that fall in the range from `to_token` up to
# `membership_snapshot_token`
membership_change_events_after_to_token = (
await self.store.get_membership_changes_for_user(
user_id,
from_key=to_token.room_key,
to_key=membership_snapshot_token,
excluded_rooms=self.rooms_to_exclude_globally,
#
# If our `to_token` is already the same or ahead of the latest room membership
# for the user, we don't need to do any "2)" fix-ups and can just straight-up
# use the room list from the snapshot as a base (nothing has changed)
membership_change_events_after_to_token = []
if not membership_snapshot_token.is_before_or_eq(to_token.room_key):
membership_change_events_after_to_token = (
await self.store.get_membership_changes_for_user(
user_id,
from_key=to_token.room_key,
to_key=membership_snapshot_token,
excluded_rooms=self.rooms_to_exclude_globally,
)
)
)

# 1) Assemble a list of the last membership events in some given ranges. Someone
# could have left and joined multiple times during the given range but we only
Expand Down
2 changes: 1 addition & 1 deletion tests/handlers/test_sliding_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ def test_only_newly_left_rooms_show_up(self) -> None:

# Leave during the from_token/to_token range (newly_left)
room_id2 = self.helper.create_room_as(user1_id, tok=user1_tok)
self.helper.leave(room_id1, user1_id, tok=user1_tok)
self.helper.leave(room_id2, user1_id, tok=user1_tok)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo in the tests which is why we didn't catch this before


after_room2_token = self.event_sources.get_current_token()

Expand Down
Loading