Skip to content

Fix background update for sliding sync (find previous membership) (v1) #17631

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

Merged
erikjohnston merged 2 commits into from
Aug 29, 2024
Merged

Fix background update for sliding sync (find previous membership) (v1) #17631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
368e26c
Revert "Use event_auth table to get previous membership"
erikjohnston Aug 29, 2024
28c4b60
Newsfile
erikjohnston Aug 29, 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/17631.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Store sliding sync per-connection state in the database.
31 changes: 19 additions & 12 deletions synapse/storage/databases/main/events_bg_updates.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1961,27 +1961,33 @@ def _find_memberships_to_update_txn(
return 0

def _find_previous_membership_txn(
txn: LoggingTransaction, event_id: str, user_id: str
txn: LoggingTransaction, room_id: str, user_id: str, stream_ordering: int
) -> Tuple[str, str]:
# Find the previous invite/knock event before the leave event. This
# is done by looking at the auth events of the invite/knock and
# finding the corresponding membership event.
# Find the previous invite/knock event before the leave event
txn.execute(
"""
SELECT m.event_id, m.membership
FROM event_auth AS a
INNER JOIN room_memberships AS m ON (a.auth_id = m.event_id)

@MadLittleMods MadLittleMods Aug 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For context on why this doesn't work, @erikjohnston ran this on matrix.org and found a leave membership where the event_auth only has two events that we don't have at all. The low number of event_auth is suspicious and really strange that we don't have the events at all. Even checking the PDU event JSON of the leave event shows it having two auth events.

According to the room_memberships table, they were previously invite

Another strange thing is that the room_memberships.event_id of the invite event doesn't match one of the event_auth of the leave event (the leave event doesn't point back to the invite).

WHERE a.event_id = ? AND m.user_id = ?
SELECT event_id, membership
FROM room_memberships
WHERE
room_id = ?
AND user_id = ?
AND event_stream_ordering < ?

@MadLittleMods MadLittleMods Aug 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@erikjohnston found another snag down the line:

Turns out room_memberships.event_stream_ordering can be null here.

We need to rely on the events.stream_ordering and do a INNER JOIN events AS e USING (room_id, event_id)

Even though the events.stream_ordering also doesn't have a NOT NULL constraint, it doesn't have any rows where this is the case (SELECT event_id FROM events WHERE stream_ordering = null;). The fact it is a nullable column is a holdover from a rename of the column.

@MadLittleMods MadLittleMods Aug 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixed in #17632

ORDER BY event_stream_ordering DESC
LIMIT 1
""",
(event_id, user_id),
(
room_id,
user_id,
stream_ordering,
),
)
row = txn.fetchone()

# We should see a corresponding previous invite/knock event
assert row is not None
previous_event_id, membership = row
event_id, membership = row

return previous_event_id, membership
return event_id, membership

# Map from (room_id, user_id) to ...
to_insert_membership_snapshots: Dict[
Expand Down Expand Up @@ -2097,8 +2103,9 @@ def _find_previous_membership_txn(
await self.db_pool.runInteraction(
"sliding_sync_membership_snapshots_bg_update._find_previous_membership",
_find_previous_membership_txn,
membership_event_id,
room_id,
user_id,
membership_event_stream_ordering,
)
)

Expand Down
10 changes: 4 additions & 6 deletions tests/storage/test_sliding_sync_tables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,9 @@ def _create_remote_invite_room_for_user(
return invite_room_id, persisted_event

def _retract_remote_invite_for_user(
self, user_id: str, remote_room_id: str, invite_event_id: str
self,
user_id: str,
remote_room_id: str,
) -> EventBase:
"""
Create a fake invite retraction for a remote room and persist it.
Expand All @@ -283,7 +285,6 @@ def _retract_remote_invite_for_user(
user_id: The person who was invited and we're going to retract the
invite for.
remote_room_id: The room ID that the invite was for.
invite_event_id: The event ID of the invite

Returns:
The persisted leave (kick) event.
Expand All @@ -297,7 +298,7 @@ def _retract_remote_invite_for_user(
"origin_server_ts": 1,
"type": EventTypes.Member,
"content": {"membership": Membership.LEAVE},
"auth_events": [invite_event_id],
"auth_events": [],
"prev_events": [],
}

Expand Down Expand Up @@ -2346,7 +2347,6 @@ def test_non_join_retracted_remote_invite(self) -> None:
remote_invite_retraction_event = self._retract_remote_invite_for_user(
user_id=user1_id,
remote_room_id=remote_invite_room_id,
invite_event_id=remote_invite_event.event_id,
)

# No one local is joined to the remote room
Expand Down Expand Up @@ -3580,15 +3580,13 @@ def test_membership_snapshots_background_update_remote_invite_rejections_and_ret
room_id_no_info_leave_event = self._retract_remote_invite_for_user(
user_id=user1_id,
remote_room_id=room_id_no_info,
invite_event_id=room_id_no_info_invite_event.event_id,
)
room_id_with_info_leave_event_response = self.helper.leave(
room_id_with_info, user1_id, tok=user1_tok
)
space_room_id_leave_event = self._retract_remote_invite_for_user(
user_id=user1_id,
remote_room_id=space_room_id,
invite_event_id=space_room_id_invite_event.event_id,
)

# Clean-up the `sliding_sync_membership_snapshots` table as if the inserts did not
Expand Down