Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix stuck notification counts on small servers #13168

Merged
merged 4 commits into from
Jul 4, 2022
Merged

Fix stuck notification counts on small servers #13168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa34e38
Fix stuck notification counts on small servers
erikjohnston Jul 4, 2022
8524344
Fix rare race causing incorrect notification counts
erikjohnston Jul 4, 2022
d157d12
Newsfile
erikjohnston Jul 4, 2022
3472063
Add missing comment
erikjohnston Jul 4, 2022
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/13168.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix unread counts for users on small servers. Introduced in v1.62.0rc1.
9 changes: 7 additions & 2 deletions synapse/storage/databases/main/event_push_actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,7 +972,12 @@ def _rotate_notifs_txn(self, txn: LoggingTransaction) -> bool:
stream_row = txn.fetchone()
if stream_row:
(offset_stream_ordering,) = stream_row
rotate_to_stream_ordering = offset_stream_ordering

# We need to bound by the current token to ensure that we handle
# out-of-order writes correctly.
rotate_to_stream_ordering = min(
offset_stream_ordering, self._stream_id_gen.get_current_token()
)
caught_up = False
else:
rotate_to_stream_ordering = self._stream_id_gen.get_current_token()
Expand Down Expand Up @@ -1004,7 +1009,7 @@ def _rotate_notifs_before_txn(
SELECT user_id, room_id, count(*) as cnt,
max(stream_ordering) as stream_ordering
FROM event_push_actions
WHERE ? <= stream_ordering AND stream_ordering < ?
WHERE ? < stream_ordering AND stream_ordering <= ?
Copy link
Member Author

Choose a reason for hiding this comment

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

Stream orderings are always queried by exclusive-inclusive bounds.

AND %s = 1
GROUP BY user_id, room_id
) AS upd
Expand Down
10 changes: 5 additions & 5 deletions tests/storage/test_event_push_actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,12 @@ def _mark_read(stream: int, depth: int) -> None:
_assert_counts(0, 0)
_inject_actions(1, PlAIN_NOTIF)
_assert_counts(1, 0)
_rotate(2)
_rotate(1)
Copy link
Member Author

Choose a reason for hiding this comment

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

The test missed the off-by-one error as we rotated at a stream ordering not associated with an event, which is not how the code works in practice.

Copy link
Member

Choose a reason for hiding this comment

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

Would this have been caught if this test didn't poke the guts of everything and created a real room and real events?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes :(

(I want to fix that but not in a RC)

_assert_counts(1, 0)

_inject_actions(3, PlAIN_NOTIF)
_assert_counts(2, 0)
_rotate(4)
_rotate(3)
_assert_counts(2, 0)

_inject_actions(5, PlAIN_NOTIF)
Expand All @@ -162,7 +162,7 @@ def _mark_read(stream: int, depth: int) -> None:
_assert_counts(0, 0)

_inject_actions(6, PlAIN_NOTIF)
_rotate(7)
_rotate(6)
_assert_counts(1, 0)

self.get_success(
Expand All @@ -178,13 +178,13 @@ def _mark_read(stream: int, depth: int) -> None:

_inject_actions(8, HIGHLIGHT)
_assert_counts(1, 1)
_rotate(9)
_rotate(8)
_assert_counts(1, 1)

# Check that adding another notification and rotating after highlight
# works.
_inject_actions(10, PlAIN_NOTIF)
_rotate(11)
_rotate(10)
_assert_counts(2, 1)

# Check that sending read receipts at different points results in the
Expand Down