From fa34e386ccfbd2de5ad9ecf5a9e72433c514f597 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 4 Jul 2022 11:40:43 +0100 Subject: [PATCH 1/4] Fix stuck notification counts on small servers There was an off-by-one error due to incorrect bounds on stream ordering. --- synapse/storage/databases/main/event_push_actions.py | 2 +- tests/storage/test_event_push_actions.py | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index 7d4754b3d37a..de6583965cb6 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -1004,7 +1004,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 <= ? AND %s = 1 GROUP BY user_id, room_id ) AS upd diff --git a/tests/storage/test_event_push_actions.py b/tests/storage/test_event_push_actions.py index 684485ae0629..852b66338795 100644 --- a/tests/storage/test_event_push_actions.py +++ b/tests/storage/test_event_push_actions.py @@ -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) _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) @@ -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( @@ -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 From 8524344009c89d3c13fc33ceef251ead6ff7cbc6 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 4 Jul 2022 11:41:28 +0100 Subject: [PATCH 2/4] Fix rare race causing incorrect notification counts We always need to upper bound the stream ordering when querying the DB, due to out of order persistence. --- synapse/storage/databases/main/event_push_actions.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index de6583965cb6..47e4a1d98caf 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -972,7 +972,9 @@ 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 + 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() From d157d12ab4f0b9863fbddad60b8df2413f9aebaa Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 4 Jul 2022 11:43:06 +0100 Subject: [PATCH 3/4] Newsfile --- changelog.d/13168.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/13168.bugfix diff --git a/changelog.d/13168.bugfix b/changelog.d/13168.bugfix new file mode 100644 index 000000000000..f462260c5943 --- /dev/null +++ b/changelog.d/13168.bugfix @@ -0,0 +1 @@ +Fix unread counts for users on small servers. Introduced in v1.62.0rc1. From 34720635f6984c0a6a929758509dcb0504bcb726 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 4 Jul 2022 13:19:45 +0100 Subject: [PATCH 4/4] Add missing comment --- synapse/storage/databases/main/event_push_actions.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/storage/databases/main/event_push_actions.py b/synapse/storage/databases/main/event_push_actions.py index 47e4a1d98caf..505616e210d6 100644 --- a/synapse/storage/databases/main/event_push_actions.py +++ b/synapse/storage/databases/main/event_push_actions.py @@ -972,6 +972,9 @@ def _rotate_notifs_txn(self, txn: LoggingTransaction) -> bool: stream_row = txn.fetchone() if stream_row: (offset_stream_ordering,) = stream_row + + # 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() )