Disable calculating unread counts unless the config flag is enabled. (#…

…13694)

This avoids doing work that will never be used (since the
resulting unread counts will never be sent in a /sync
response).

The negative of doing this is that unread counts will be
incorrect when the feature is initially enabled.

changelog.d/13694.bugfix

@@ -0,0 +1 @@
+Fix a bug introduced in Synapse v1.20.0 that would cause the unstable unread counts from [MSC2654](https://github.com/matrix-org/matrix-spec-proposals/pull/2654) to be calculated even if the feature is disabled.

synapse/config/experimental.py

@@ -71,6 +71,9 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
         self.msc3720_enabled: bool = experimental.get("msc3720_enabled", False)
 
         # MSC2654: Unread counts
+        #
+        # Note that enabling this will result in an incorrect unread count for
+        # previously calculated push actions.
         self.msc2654_enabled: bool = experimental.get("msc2654_enabled", False)
 
         # MSC2815 (allow room moderators to view redacted event content)

synapse/push/bulk_push_rule_evaluator.py

@@ -262,7 +262,12 @@ async def action_for_event_by_user(
             # This can happen due to out of band memberships
             return
 
-        count_as_unread = _should_count_as_unread(event, context)
+        # Disable counting as unread unless the experimental configuration is
+        # enabled, as it can cause additional (unwanted) rows to be added to the
+        # event_push_actions table.
+        count_as_unread = False
+        if self.hs.config.experimental.msc2654_enabled:
+            count_as_unread = _should_count_as_unread(event, context)
 
         rules_by_user = await self._get_rules_for_event(event)
         actions_by_user: Dict[str, Collection[Union[Mapping, str]]] = {}

tests/storage/test_event_push_actions.py

@@ -67,9 +67,7 @@ def test_count_aggregation(self) -> None:
 
         last_event_id: str
 
-        def _assert_counts(
-            noitf_count: int, unread_count: int, highlight_count: int
-        ) -> None:
+        def _assert_counts(noitf_count: int, highlight_count: int) -> None:
             counts = self.get_success(
                 self.store.db_pool.runInteraction(
                     "get-unread-counts",
@@ -82,7 +80,7 @@ def _assert_counts(
                 counts,
                 NotifCounts(
                     notify_count=noitf_count,
-                    unread_count=unread_count,
+                    unread_count=0,
                     highlight_count=highlight_count,
                 ),
             )
@@ -112,27 +110,27 @@ def _mark_read(event_id: str) -> None:
                 )
             )
 
-        _assert_counts(0, 0, 0)
+        _assert_counts(0, 0)
         _create_event()
-        _assert_counts(1, 1, 0)
+        _assert_counts(1, 0)
         _rotate()
-        _assert_counts(1, 1, 0)
+        _assert_counts(1, 0)
 
         event_id = _create_event()
-        _assert_counts(2, 2, 0)
+        _assert_counts(2, 0)
         _rotate()
-        _assert_counts(2, 2, 0)
+        _assert_counts(2, 0)
 
         _create_event()
         _mark_read(event_id)
-        _assert_counts(1, 1, 0)
+        _assert_counts(1, 0)
 
         _mark_read(last_event_id)
-        _assert_counts(0, 0, 0)
+        _assert_counts(0, 0)
 
         _create_event()
         _rotate()
-        _assert_counts(1, 1, 0)
+        _assert_counts(1, 0)
 
         # Delete old event push actions, this should not affect the (summarised) count.
         #
@@ -151,35 +149,35 @@ def _mark_read(event_id: str) -> None:
             )
         )
         self.assertEqual(result, [])
-        _assert_counts(1, 1, 0)
+        _assert_counts(1, 0)
 
         _mark_read(last_event_id)
-        _assert_counts(0, 0, 0)
+        _assert_counts(0, 0)
 
         event_id = _create_event(True)
-        _assert_counts(1, 1, 1)
+        _assert_counts(1, 1)
         _rotate()
-        _assert_counts(1, 1, 1)
+        _assert_counts(1, 1)
 
         # Check that adding another notification and rotating after highlight
         # works.
         _create_event()
         _rotate()
-        _assert_counts(2, 2, 1)
+        _assert_counts(2, 1)
 
         # Check that sending read receipts at different points results in the
         # right counts.
         _mark_read(event_id)
-        _assert_counts(1, 1, 0)
+        _assert_counts(1, 0)
         _mark_read(last_event_id)
-        _assert_counts(0, 0, 0)
+        _assert_counts(0, 0)
 
         _create_event(True)
-        _assert_counts(1, 1, 1)
+        _assert_counts(1, 1)
         _mark_read(last_event_id)
-        _assert_counts(0, 0, 0)
+        _assert_counts(0, 0)
         _rotate()
-        _assert_counts(0, 0, 0)
+        _assert_counts(0, 0)
 
     def test_find_first_stream_ordering_after_ts(self) -> None:
         def add_event(so: int, ts: int) -> None: