Reorder rooms by read receipts to update most important rooms first#19613
Reorder rooms by read receipts to update most important rooms first#19613neilisfragile wants to merge 8 commits into
Conversation
| all_room_ids = await self.store.get_rooms_for_user(target_user.to_string()) | ||
|
|
||
| # Get the user's latest read receipts for all rooms | ||
| user_receipts = await self.store.get_receipts_for_user_with_orderings( | ||
| target_user.to_string(), | ||
| [ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE], | ||
| ) | ||
|
|
||
| # Sort rooms by most recent read receipt (highest stream_ordering first), | ||
| # with fallback to alphabetical ordering for rooms without receipts | ||
| def sort_key(room_id: str) -> tuple[int, str]: | ||
| if room_id in user_receipts: | ||
| # Rooms with receipts: sort by stream_ordering (descending) then by room_id | ||
| return (-user_receipts[room_id]["stream_ordering"], room_id) | ||
| else: | ||
| # Rooms without receipts: sort alphabetically after all rooms with receipts | ||
| return (0, room_id) | ||
|
|
||
| room_ids = sorted(all_room_ids, key=sort_key) |
There was a problem hiding this comment.
The important consideration is that once the room ordering is determined, it is never updated, even over a restart. This ensures all rooms are updated eventually.
I don't think this is true?
From what I can see the list will be computed again on the next call to the task after a restart for example, and we hence may miss rooms if they were read in the meantime.
I think you should:
- pre compute the ordered list of room ids in
_update_join_statesbefore scheduling the task - schedule the task with the list stored in
task.params - add a test for that :)
test_background_update_room_membership_resume_after_restartis simulating a restart so you should probably take that as an example - put less trust in Claude output
| @@ -0,0 +1 @@ | |||
| Reorder rooms by read receipts to update most important rooms first. | |||
There was a problem hiding this comment.
Removing from the review queue until #19613 (comment) gets some attention ⏩
…ing_by_read_receipt
…eived_display_name_performance
…eived_display_name_performance
|
The PR is consistently failing on the worker version of the complement test TestThreadedReceipts It is unclear why the changes are causing the failure, but even though it is on the flaky tests list, I think there is something to dig into. |
|
(for reference, the tests passed on the last run) |
Yes, and I can't yet reproduce it locally either. There are still one or two things I want to try before just blaming a flaky test. |
3 participants
Problem to solve
Updating profile information, such as display name or avatar, requires updating state events in every room the user is part of. For large accounts, this can be a time-consuming operation. Historically, making an update led to a timeout and an ugly/confusing error to the user.
#17074 tried to fix this problem by returning immediately and then running the updates asynchronously in the background. The PR stalled due to time constraints, and the same problem was addressed and merged in #19311
#19311 fixed the timeout issue, but for large accounts, the experience is still jarring because rooms do not update immediately, so from the user's perspective, the update appears to have failed.
One aspect of #17074 that did not make it to #19311 is to order the room update by likely importance to the user.
This PR tries to address the importance issue by ordering the room list by read receipt rather than alphabetically. Since we're now processing rooms in the order of last read receipt, it will appear to be better because the rooms that the user looks at will be updated and hide the fact that we're still working on things in the background.
The important consideration is that once the room ordering is determined, it is never updated, even over a restart. This ensures all rooms are updated eventually.
Long term we might try to fix this with https://github.com/matrix-org/matrix-spec-proposals/blob/kegan/profile-changes/proposals/4218-profile-change-perf.md
Note, I had a first attempt to fix this in #19311, which, in retrospect, was overly complicated and tried to maintain two orderings (stream ordering and alphabetical).
Many thanks to @MatMaul for #19311
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.This PR is Claude-assisted, though line by line, I am confident in its operation.