Prevent MSC4354 Sticky Events being sticky when they were redacted in the same persistence batch. (#20322)

Part of: MSC4354
Part of: #19641
Follows: #20232 (which applies already-persisted redactions to Sticky
Events being persisted)


In some cases we batch events together during persistence.

If one of the events in the batch is a redaction of another event, we
should apply it, which provides the same benefit as #20232 (namely: we
avoid potentially dodgy event content being persisted & the event
naturally loses its stickiness
so is not written to `sticky_events`)

---------

Signed-off-by: Olivier 'reivilibre <oliverw@matrix.org>

changelog.d/20322.bugfix

@@ -0,0 +1 @@
+Prevent [MSC4354 Sticky Events](https://github.com/matrix-org/matrix-spec-proposals/pull/4354) being sticky when they were redacted in the same persistence batch.

synapse/storage/databases/main/events.py

@@ -250,6 +250,16 @@ class NewEventChainLinks:
     links: list[tuple[int, int]] = attr.Factory(list)
 
 
+@attr.s(slots=True, frozen=True, auto_attribs=True)
+class _RedactionInfo:
+    """Information about a redaction. For internal use (`_apply_existing_redaction_txn`)."""
+
+    event_id: str
+    redacts_event_id: str
+    event_sender: str
+    event_needs_v3_recheck: bool
+
+
 class PersistEventsStore:
     """Contains all the functions for writing events to the database.
 
@@ -2924,6 +2934,10 @@ class PersistEventsStore:
         If we have any redactions for the events we're about to persist,
         pre-applies those if they are appropriate.
 
+        The redactions may either have been persisted previously
+        (e.g. we received the redaction over federation before the event it redacts)
+        or be part of this same batch of events.
+
         Redaction events themselves won't be redacted immediately, to avoid
         breaking circular redactions (which are tested in `test_circular_redaction`).
 
@@ -2938,51 +2952,89 @@ class PersistEventsStore:
         Returns a copy of the `events_and_contexts` lists with redactions applied.
         """
 
+        assert all(ev.room_id == room_id for ev, _ in events_and_contexts)
+
         # Get all the event IDs, whilst also filtering out redaction events
         # which we don't want to redact immediately anyway
-        event_ids = [
+        event_ids = {
             ev.event_id
             for ev, _ in events_and_contexts
             if ev.type != EventTypes.Redaction
-        ]
+        }
 
         if not event_ids:
             # nothing to do here
             return events_and_contexts
 
+        def get_persisted_redactions() -> list[_RedactionInfo]:
+            """
+            Fetches the already-persisted redactions affecting the events in this batch.
+            """
+            event_id_in_list_clause, event_id_in_list_args = make_in_list_sql_clause(
+                txn.database_engine,
+                "redactions.redacts",
+                event_ids,
+            )
+            txn.execute(
+                f"""
+                SELECT redactions.event_id, redactions.redacts, redaction_events.sender, redactions.recheck
+                FROM redactions
+                INNER JOIN events AS redaction_events
+                    ON redactions.event_id = redaction_events.event_id
+                    AND redaction_events.room_id = ?
+                WHERE {event_id_in_list_clause}
+                """,
+                (room_id, *event_id_in_list_args),
+            )
+            return [
+                _RedactionInfo(
+                    event_id=redaction_event_id,
+                    redacts_event_id=redaction_redacts_event_id,
+                    event_sender=redaction_event_sender,
+                    event_needs_v3_recheck=bool(redaction_event_needs_v3_recheck),
+                )
+                for (
+                    redaction_event_id,
+                    redaction_redacts_event_id,
+                    redaction_event_sender,
+                    redaction_event_needs_v3_recheck,
+                ) in txn
+            ]
+
+        def get_redactions_in_batch() -> list[_RedactionInfo]:
+            """
+            Picks out redactions _within this batch_ that affect events also in this batch.
+            """
+            return [
+                _RedactionInfo(
+                    event_id=ev.event_id,
+                    redacts_event_id=ev.redacts,
+                    event_sender=ev.sender,
+                    event_needs_v3_recheck=ev.internal_metadata.need_to_check_redaction(),
+                )
+                for ev, ctx in events_and_contexts
+                if ev.type == EventTypes.Redaction
+                and ev.redacts is not None
+                and ev.redacts in event_ids
+                and not ctx.rejected
+            ]
+
         events_by_id = {ev.event_id: ev for ev, _ in events_and_contexts}
-        event_id_in_list_clause, event_id_in_list_args = make_in_list_sql_clause(
-            txn.database_engine,
-            "redactions.redacts",
-            event_ids,
-        )
-        txn.execute(
-            f"""
-            SELECT redactions.event_id, redactions.redacts, redaction_events.sender, redactions.recheck
-            FROM redactions
-            INNER JOIN events AS redaction_events
-                ON redactions.event_id = redaction_events.event_id
-                AND redaction_events.room_id = ?
-            WHERE {event_id_in_list_clause}
-            """,
-            (room_id, *event_id_in_list_args),
-        )
 
         # map from redacted event ID to the event ID that redacts it
         redacted_event_id_map = {}
-        for (
-            redaction_event_id,
-            redaction_redacts_event_id,
-            redaction_event_sender,
-            redaction_event_needs_v3_recheck,
-        ) in txn:
-            if redaction_redacts_event_id in redacted_event_id_map:
+
+        for redaction in itertools.chain(
+            get_persisted_redactions(),
+            get_redactions_in_batch(),
+        ):
+            if redaction.redacts_event_id in redacted_event_id_map:
                 # Already redacted
                 continue
 
-            event = events_by_id[redaction_redacts_event_id]
+            event = events_by_id[redaction.redacts_event_id]
             if (
-                redaction_event_needs_v3_recheck
+                redaction.event_needs_v3_recheck
                 # Normally v1/v2 don't ever need rechecks because the checks are part of auth rules.
                 # However the `recheck` column was only recently introduced as a retrofit,
                 # with a database-level default of `true`.
@@ -2991,27 +3043,27 @@ class PersistEventsStore:
                 and room_version.event_format != EventFormatVersions.ROOM_V1_V2
             ):
                 # Apply the same logic as `_maybe_redact_event_row`
-                if get_domain_from_id(redaction_event_sender) != get_domain_from_id(
+                if get_domain_from_id(redaction.event_sender) != get_domain_from_id(
                     event.sender
                 ):
                     # Sender servers don't match, so the event isn't actually redacted
                     logger.debug(
                         "redaction of %s by %s skipped as it required a v3 recheck and sender servers differ: %r != %r",
-                        redaction_redacts_event_id,
-                        redaction_event_id,
-                        redaction_event_sender,
+                        redaction.redacts_event_id,
+                        redaction.event_id,
+                        redaction.event_sender,
                         event.sender,
                     )
                     continue
 
                 # Unlike `_maybe_redact_event_row`, we _don't_ mutate the cached instance of the redaction event to set
-                # `recheck_redaction` to False here, as we don't actually load the redaction out of the database.
+                # `recheck_redaction` to False here, as we don't necessarily have the redaction event to hand.
 
-            redacted_event_id_map[redaction_redacts_event_id] = redaction_event_id
+            redacted_event_id_map[redaction.redacts_event_id] = redaction.event_id
             logger.debug(
                 "%r redacted at persistence time by %r",
-                redaction_redacts_event_id,
-                redaction_event_id,
+                redaction.redacts_event_id,
+                redaction.event_id,
             )
 
         out: list[EventPersistencePair] = []

tests/storage/test_sticky_events.py

@@ -14,6 +14,7 @@ import sqlite3
 from http import HTTPStatus
 
 from immutabledict import immutabledict
+from parameterized import parameterized
 
 from twisted.internet.testing import MemoryReactor
 
@@ -469,6 +470,79 @@ class StickyEventsTestCase(unittest.HomeserverTestCase):
         # ...nor shown to clients down sync.
         self.assertEqual(self._get_visible_sticky_event_ids(), set())
 
+    @parameterized.expand([(True,), (False,)])
+    def test_redacted_in_same_batch_not_tracked(
+        self, flip_order_in_batch: bool
+    ) -> None:
+        """
+        Tests that a sticky event which is persisted in the same batch as a redaction of it
+        is not sent down to clients over sync and is not added to the `sticky_events` table.
+        """
+        user2_id = self.register_user("user2", "pass")
+        user2_tok = self.login(user2_id, "pass")
+        self.helper.join(self.room_id, user2_id, tok=user2_tok)
+
+        persist_controller = self.hs.get_storage_controllers().persistence
+        assert persist_controller is not None
+
+        sticky_event, sticky_event_context = self.get_success(
+            create_event(
+                self.hs,
+                room_id=self.room_id,
+                sender=user2_id,
+                type=EventTypes.Message,
+                content={"body": "sticky", "msgtype": "m.text"},
+                # Corresponds to StickyEvent.EVENT_FIELD_NAME
+                msc4354_sticky=StickyEventField(
+                    duration_ms=Duration(minutes=1).as_millis()
+                ),
+            )
+        )
+        redaction_event, redaction_event_context = self.get_success(
+            create_event(
+                self.hs,
+                room_id=self.room_id,
+                sender=user2_id,
+                type=EventTypes.Redaction,
+                content={
+                    "reason": "nothing here but us trees",
+                    "redacts": sticky_event.event_id,
+                },
+            )
+        )
+
+        # Persist both events in a single batch.
+        # Test both orders of events to make sure it doesn't affect the outcome.
+        if flip_order_in_batch:
+            batch = [
+                (redaction_event, redaction_event_context),
+                (sticky_event, sticky_event_context),
+            ]
+        else:
+            batch = [
+                (sticky_event, sticky_event_context),
+                (redaction_event, redaction_event_context),
+            ]
+        self.get_success(persist_controller.persist_events(batch))
+
+        # The event should have been persisted in its redacted form.
+        event = self.get_success(self.store.get_event(sticky_event.event_id))
+        self.assertEqual(event.internal_metadata.redacted_by, redaction_event.event_id)
+        self.assertEqual(event.content, {})
+        self.assertIsNone(event.sticky_duration())
+
+        # Since it is redacted, it must not have been added to the sticky_events
+        # table...
+        sticky_events = self.get_success(
+            self.store.db_pool.simple_select_list(
+                table="sticky_events", keyvalues=None, retcols=("event_id",)
+            )
+        )
+        self.assertEqual(sticky_events, [])
+
+        # ...nor shown to clients down sync.
+        self.assertEqual(self._get_visible_sticky_event_ids(), set())
+
     def test_soft_failed_events_are_tracked(self) -> None:
         """
         Tests that sticky events marked as soft_failed ARE inserted