Skip to content

Commit

Permalink
Revert back to sorting by the latest event
Browse files Browse the repository at this point in the history 
See #17395 (comment)

This better ensures that clients receive any new events
regardless of what they are.
  • Loading branch information
@MadLittleMods
MadLittleMods committed Jul 4, 2024
1 parent 812a6e0 commit b59ccf7
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 83 deletions.
98 changes: 30 additions & 68 deletions synapse/handlers/sliding_sync.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,54 +134,10 @@ def copy_and_replace(self, **kwds: Any) -> "_RoomMembershipForUser":
return attr.evolve(self, **kwds)


@attr.s(slots=True, frozen=True, auto_attribs=True)
class _SortedRoomMembershipForUser(_RoomMembershipForUser):
"""
Same as `_RoomMembershipForUser` but with an additional `bump_stamp` attribute.
Attributes:
bump_stamp: The `stream_ordering` of the last event according to the
`bump_event_types`. This helps clients sort more readily without them needing to
pull in a bunch of the timeline to determine the last activity.
`bump_event_types` is a thing because for example, we don't want display name
changes to mark the room as unread and bump it to the top. For encrypted rooms,
we just have to consider any activity as a bump because we can't see the content
and the client has to figure it out for themselves.
"""

bump_stamp: int

@classmethod
def from_room_membership_for_user(
cls,
rooms_membership_for_user: _RoomMembershipForUser,
*,
bump_stamp: int,
) -> "_SortedRoomMembershipForUser":
"""
Copy from the given `_RoomMembershipForUser`, sprinkle on the specific
`_SortedRoomMembershipForUser` fields to create a new
`_SortedRoomMembershipForUser`.
"""

return cls(
room_id=rooms_membership_for_user.room_id,
event_id=rooms_membership_for_user.event_id,
event_pos=rooms_membership_for_user.event_pos,
membership=rooms_membership_for_user.membership,
sender=rooms_membership_for_user.sender,
newly_joined=rooms_membership_for_user.newly_joined,
bump_stamp=bump_stamp,
)

def copy_and_replace(self, **kwds: Any) -> "_SortedRoomMembershipForUser":
return attr.evolve(self, **kwds)


@attr.s(slots=True, frozen=True, auto_attribs=True)
class _RelevantRoomEntry:
room_sync_config: RoomSyncConfig
room_membership_for_user: _SortedRoomMembershipForUser
room_membership_for_user: _RoomMembershipForUser


class SlidingSyncHandler:
Expand Down Expand Up @@ -847,7 +803,7 @@ async def sort_rooms(
self,
sync_room_map: Dict[str, _RoomMembershipForUser],
to_token: StreamToken,
) -> List[_SortedRoomMembershipForUser]:
) -> List[_RoomMembershipForUser]:
"""
Sort by `stream_ordering` of the last event that the user should see in the
room. `stream_ordering` is unique so we get a stable sort.
Expand All @@ -861,31 +817,27 @@ async def sort_rooms(
A sorted list of room IDs by `stream_ordering` along with membership information.
"""

sorted_sync_rooms: List[_SortedRoomMembershipForUser] = []

# Assemble a map of room ID to the `stream_ordering` of the last activity that the
# user should see in the room (<= `to_token`)
last_activity_in_room_map: Dict[str, int] = {}
for room_id, room_for_user in sync_room_map.items():
# If they are fully-joined to the room, let's find the latest activity
# at/before the `to_token`.
if room_for_user.membership == Membership.JOIN:
last_event_result = (
await self.store.get_last_event_pos_in_room_before_stream_ordering(
room_id, to_token.room_key, event_types=DEFAULT_BUMP_EVENT_TYPES
room_id, to_token.room_key
)
)

# By default, just choose the membership event position
event_pos = room_for_user.event_pos
# But if we found a bump event, use that instead
if last_event_result is not None:
_, event_pos = last_event_result
# If the room has no events at/before the `to_token`, this is probably a
# mistake in the code that generates the `sync_room_map` since that should
# only give us rooms that the user had membership in during the token range.
assert last_event_result is not None

sorted_sync_rooms.append(
_SortedRoomMembershipForUser.from_room_membership_for_user(
room_for_user, bump_stamp=event_pos.stream
)
)
_, event_pos = last_event_result

last_activity_in_room_map[room_id] = event_pos.stream
else:
# Otherwise, if the user has left/been invited/knocked/been banned from
# a room, they shouldn't see anything past that point.
Expand All @@ -894,16 +846,12 @@ async def sort_rooms(
# invited/knocked cases if for example the room has
# `invite`/`world_readable` history visibility, see
# https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1653045932
sorted_sync_rooms.append(
_SortedRoomMembershipForUser.from_room_membership_for_user(
room_for_user, bump_stamp=room_for_user.event_pos.stream
)
)
last_activity_in_room_map[room_id] = room_for_user.event_pos.stream

return sorted(
sorted_sync_rooms,
sync_room_map.values(),
# Sort by the last activity (stream_ordering) in the room
key=lambda room_info: room_info.bump_stamp,
key=lambda room_info: last_activity_in_room_map[room_info.room_id],
# We want descending order
reverse=True,
)
Expand All @@ -913,7 +861,7 @@ async def get_room_sync_data(
user: UserID,
room_id: str,
room_sync_config: RoomSyncConfig,
room_membership_for_user_at_to_token: _SortedRoomMembershipForUser,
room_membership_for_user_at_to_token: _RoomMembershipForUser,
from_token: Optional[StreamToken],
to_token: StreamToken,
) -> SlidingSyncResult.RoomResult:
Expand Down Expand Up @@ -1102,6 +1050,20 @@ async def get_room_sync_data(
# state reset happened. Perhaps we should indicate this by setting `initial:
# True` and empty `required_state`.

# Figure out the last bump event in the room
last_bump_event_result = (
await self.store.get_last_event_pos_in_room_before_stream_ordering(
room_id, to_token.room_key, event_types=DEFAULT_BUMP_EVENT_TYPES
)
)

# By default, just choose the membership event position
bump_stamp = room_membership_for_user_at_to_token.event_pos.stream
# But if we found a bump event, use that instead
if last_bump_event_result:
_, bump_event_pos = last_bump_event_result
bump_stamp = bump_event_pos.stream

return SlidingSyncResult.RoomResult(
# TODO: Dummy value
name=None,
Expand All @@ -1123,7 +1085,7 @@ async def get_room_sync_data(
stripped_state=stripped_state,
prev_batch=prev_batch_token,
limited=limited,
bump_stamp=room_membership_for_user_at_to_token.bump_stamp,
bump_stamp=bump_stamp,
# TODO: Dummy values
joined_count=0,
invited_count=0,
Expand Down
12 changes: 7 additions & 5 deletions tests/handlers/test_sliding_sync.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2417,7 +2417,8 @@ def test_activity_after_xxx(self, room1_membership: str) -> None:

def test_default_bump_event_types(self) -> None:
"""
Test that we only consider `bump_event_types` when sorting rooms.
Test that we only consider the *latest* event in the room when sorting (not
`bump_event_types`).
"""
user1_id = self.register_user("user1", "pass")
user1_tok = self.login(user1_id, "pass")
Expand All @@ -2433,8 +2434,8 @@ def test_default_bump_event_types(self) -> None:
)
self.helper.send(room_id2, "message in room2", tok=user1_tok)

# Send a reaction in room1 but it shouldn't affect the sort order
# because reactions are not part of the `DEFAULT_BUMP_EVENT_TYPES`
# Send a reaction in room1 which isn't in `DEFAULT_BUMP_EVENT_TYPES` but we only
# care about sorting by the *latest* event in the room.
self.helper.send_event(
room_id1,
type=EventTypes.Reaction,
Expand Down Expand Up @@ -2469,6 +2470,7 @@ def test_default_bump_event_types(self) -> None:

self.assertEqual(
[room_membership.room_id for room_membership in sorted_sync_rooms],
# room2 sorts before room1 because reactions don't bump the room
[room_id2, room_id1],
# room1 sorts before room2 because it has the latest event (the reaction).
# We only care about the *latest* event in the room.
[room_id1, room_id2],
)
22 changes: 12 additions & 10 deletions tests/rest/client/test_sync.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2010,27 +2010,29 @@ def test_rooms_bump_stamp(self) -> None:
{
"op": "SYNC",
"range": [0, 1],
# room2 sorts before room1 because reactions don't bump the room
"room_ids": [room_id2, room_id1],
# room1 sorts before room2 because it has the latest event (the
# reaction)
"room_ids": [room_id1, room_id2],
}
],
channel.json_body["lists"]["foo-list"],
)

# Make sure the `bump_stamp` for room2 is correct
self.assertEqual(
channel.json_body["rooms"][room_id2]["bump_stamp"],
event_pos2.stream,
channel.json_body["rooms"][room_id2],
)

# Make sure the `bump_stamp` for room2 is correct
# The `bump_stamp` for room1 should point at the latest message (not the
# reaction since it's not one of the `DEFAULT_BUMP_EVENT_TYPES`)
self.assertEqual(
channel.json_body["rooms"][room_id1]["bump_stamp"],
event_pos1.stream,
channel.json_body["rooms"][room_id1],
)

# The `bump_stamp` for room2 should point at the latest message
self.assertEqual(
channel.json_body["rooms"][room_id2]["bump_stamp"],
event_pos2.stream,
channel.json_body["rooms"][room_id2],
)

def test_rooms_newly_joined_incremental_sync(self) -> None:
"""
Test that when we make an incremental sync with a `newly_joined` `rooms`, we are
Expand Down

0 comments on commit b59ccf7

Please sign in to comment.