From a734efe788644505d1e5bbf479d87416395e855a Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Fri, 25 Feb 2022 14:30:08 -0500
Subject: [PATCH 01/12] Test behavior of /relations for a redacted relation.

---
 tests/rest/client/test_relations.py | 70 ++++++++++++++++++++++++-----
 1 file changed, 60 insertions(+), 10 deletions(-)

diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py
index a087cd7b2149..4c5fe8ef8fc3 100644
--- a/tests/rest/client/test_relations.py
+++ b/tests/rest/client/test_relations.py
@@ -1358,8 +1358,12 @@ def _redact(self, event_id: str) -> None:
         self.assertEqual(200, channel.code, channel.json_body)
 
     def test_redact_relation_annotation(self) -> None:
-        """Test that annotations of an event are properly handled after the
+        """
+        Test that annotations of an event are properly handled after the
         annotation is redacted.
+
+        The redacted relation should not be included in bundled aggregations or
+        the response to relations.
         """
         channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", "a")
         self.assertEqual(200, channel.code, channel.json_body)
@@ -1369,11 +1373,34 @@ def test_redact_relation_annotation(self) -> None:
             RelationTypes.ANNOTATION, "m.reaction", "a", access_token=self.user2_token
         )
         self.assertEqual(200, channel.code, channel.json_body)
+        unredacted_event_id = channel.json_body["event_id"]
 
         # Redact one of the reactions.
         self._redact(to_redact_event_id)
 
-        # Ensure that the aggregations are correct.
+        # The unredacted relation should still exist.
+        channel = self.make_request(
+            "GET",
+            f"/_matrix/client/unstable/rooms/{self.room}/relations/{self.parent_id}",
+            access_token=self.user_token,
+        )
+        self.assertEquals(200, channel.code, channel.json_body)
+        event_ids = [ev["event_id"] for ev in channel.json_body["chunk"]]
+        self.assertEquals(event_ids, [unredacted_event_id])
+
+        # The unredacted relation should appear in the bundled aggregation.
+        channel = self.make_request(
+            "GET",
+            f"/_matrix/client/unstable/rooms/{self.room}/event/{self.parent_id}",
+            access_token=self.user_token,
+        )
+        self.assertEquals(200, channel.code, channel.json_body)
+        self.assertEquals(
+            channel.json_body["unsigned"]["m.relations"]["m.annotation"],
+            {"chunk": [{"type": "m.reaction", "key": "a", "count": 1}]},
+        )
+
+        # The unredacted aggregation should still exist.
         channel = self.make_request(
             "GET",
             f"/_matrix/client/unstable/rooms/{self.room}/aggregations/{self.parent_id}",
@@ -1406,8 +1433,7 @@ def test_redact_relation_edit(self) -> None:
         # Check the relation is returned
         channel = self.make_request(
             "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/relations"
-            f"/{self.parent_id}/m.replace/m.room.message",
+            f"/_matrix/client/unstable/rooms/{self.room}/relations/{self.parent_id}",
             access_token=self.user_token,
         )
         self.assertEqual(200, channel.code, channel.json_body)
@@ -1418,19 +1444,25 @@ def test_redact_relation_edit(self) -> None:
         # Redact the original event
         self._redact(self.parent_id)
 
-        # Try to check for remaining m.replace relations
+        # The relations are not returned.
         channel = self.make_request(
             "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/relations"
-            f"/{self.parent_id}/m.replace/m.room.message",
+            f"/_matrix/client/unstable/rooms/{self.room}/relations/{self.parent_id}",
             access_token=self.user_token,
         )
         self.assertEqual(200, channel.code, channel.json_body)
-
-        # Check that no relations are returned
         self.assertIn("chunk", channel.json_body)
         self.assertEqual(channel.json_body["chunk"], [])
 
+        # The relations should not appear in the bundled aggregation.
+        channel = self.make_request(
+            "GET",
+            f"/_matrix/client/unstable/rooms/{self.room}/event/{self.parent_id}",
+            access_token=self.user_token,
+        )
+        self.assertEquals(200, channel.code, channel.json_body)
+        self.assertNotIn("m.relations", channel.json_body["unsigned"])
+
     def test_redact_parent(self) -> None:
         """Test that annotations of an event are redacted when the original event
         is redacted.
@@ -1442,6 +1474,25 @@ def test_redact_parent(self) -> None:
         # Redact the original event.
         self._redact(self.parent_id)
 
+        # The relations are not returned.
+        channel = self.make_request(
+            "GET",
+            f"/_matrix/client/unstable/rooms/{self.room}/relations/{self.parent_id}",
+            access_token=self.user_token,
+        )
+        self.assertEquals(200, channel.code, channel.json_body)
+        self.assertIn("chunk", channel.json_body)
+        self.assertEqual(channel.json_body["chunk"], [])
+
+        # The relations should not appear in the bundled aggregation.
+        channel = self.make_request(
+            "GET",
+            f"/_matrix/client/unstable/rooms/{self.room}/event/{self.parent_id}",
+            access_token=self.user_token,
+        )
+        self.assertEquals(200, channel.code, channel.json_body)
+        self.assertNotIn("m.relations", channel.json_body["unsigned"])
+
         # Check that aggregations returns zero
         channel = self.make_request(
             "GET",
@@ -1449,6 +1500,5 @@ def test_redact_parent(self) -> None:
             access_token=self.user_token,
         )
         self.assertEqual(200, channel.code, channel.json_body)
-
         self.assertIn("chunk", channel.json_body)
         self.assertEqual(channel.json_body["chunk"], [])

From 510b919f6cfe6a851716609c0dc6758188360c58 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Mar 2022 11:45:37 -0500
Subject: [PATCH 02/12] De-duplicate code to request relations and bundled
 aggregations.

---
 tests/rest/client/test_relations.py | 99 ++++++++++++-----------------
 1 file changed, 40 insertions(+), 59 deletions(-)

diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py
index 4c5fe8ef8fc3..1dd47aa0e6c2 100644
--- a/tests/rest/client/test_relations.py
+++ b/tests/rest/client/test_relations.py
@@ -1357,6 +1357,36 @@ def _redact(self, event_id: str) -> None:
         )
         self.assertEqual(200, channel.code, channel.json_body)
 
+    def _make_relation_requests(self) -> Tuple[List[str], JsonDict]:
+        """
+        Makes requests and ensures they result in a 200 response, returns a
+        tuple of results:
+
+        1. `/relations` -> Returns a list of event IDs.
+        2. `/event` -> Returns the response's m.relations field (from unsigned),
+            if it exists.
+        """
+
+        # Request the relations of the event.
+        channel = self.make_request(
+            "GET",
+            f"/_matrix/client/unstable/rooms/{self.room}/relations/{self.parent_id}",
+            access_token=self.user_token,
+        )
+        self.assertEquals(200, channel.code, channel.json_body)
+        event_ids = [ev["event_id"] for ev in channel.json_body["chunk"]]
+
+        # Fetch the bundled aggregations of the event.
+        channel = self.make_request(
+            "GET",
+            f"/_matrix/client/unstable/rooms/{self.room}/event/{self.parent_id}",
+            access_token=self.user_token,
+        )
+        self.assertEquals(200, channel.code, channel.json_body)
+        bundled_relations = channel.json_body["unsigned"].get("m.relations", {})
+
+        return event_ids, bundled_relations
+
     def test_redact_relation_annotation(self) -> None:
         """
         Test that annotations of an event are properly handled after the
@@ -1379,24 +1409,10 @@ def test_redact_relation_annotation(self) -> None:
         self._redact(to_redact_event_id)
 
         # The unredacted relation should still exist.
-        channel = self.make_request(
-            "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/relations/{self.parent_id}",
-            access_token=self.user_token,
-        )
-        self.assertEquals(200, channel.code, channel.json_body)
-        event_ids = [ev["event_id"] for ev in channel.json_body["chunk"]]
+        event_ids, relations = self._make_relation_requests()
         self.assertEquals(event_ids, [unredacted_event_id])
-
-        # The unredacted relation should appear in the bundled aggregation.
-        channel = self.make_request(
-            "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/event/{self.parent_id}",
-            access_token=self.user_token,
-        )
-        self.assertEquals(200, channel.code, channel.json_body)
         self.assertEquals(
-            channel.json_body["unsigned"]["m.relations"]["m.annotation"],
+            relations["m.annotation"],
             {"chunk": [{"type": "m.reaction", "key": "a", "count": 1}]},
         )
 
@@ -1431,37 +1447,16 @@ def test_redact_relation_edit(self) -> None:
         self.assertEqual(200, channel.code, channel.json_body)
 
         # Check the relation is returned
-        channel = self.make_request(
-            "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/relations/{self.parent_id}",
-            access_token=self.user_token,
-        )
-        self.assertEqual(200, channel.code, channel.json_body)
-
-        self.assertIn("chunk", channel.json_body)
-        self.assertEqual(len(channel.json_body["chunk"]), 1)
+        event_ids, _ = self._make_relation_requests()
+        self.assertEqual(len(event_ids), 1)
 
         # Redact the original event
         self._redact(self.parent_id)
 
         # The relations are not returned.
-        channel = self.make_request(
-            "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/relations/{self.parent_id}",
-            access_token=self.user_token,
-        )
-        self.assertEqual(200, channel.code, channel.json_body)
-        self.assertIn("chunk", channel.json_body)
-        self.assertEqual(channel.json_body["chunk"], [])
-
-        # The relations should not appear in the bundled aggregation.
-        channel = self.make_request(
-            "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/event/{self.parent_id}",
-            access_token=self.user_token,
-        )
-        self.assertEquals(200, channel.code, channel.json_body)
-        self.assertNotIn("m.relations", channel.json_body["unsigned"])
+        event_ids, relations = self._make_relation_requests()
+        self.assertEqual(len(event_ids), 0)
+        self.assertEqual(relations, {})
 
     def test_redact_parent(self) -> None:
         """Test that annotations of an event are redacted when the original event
@@ -1475,23 +1470,9 @@ def test_redact_parent(self) -> None:
         self._redact(self.parent_id)
 
         # The relations are not returned.
-        channel = self.make_request(
-            "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/relations/{self.parent_id}",
-            access_token=self.user_token,
-        )
-        self.assertEquals(200, channel.code, channel.json_body)
-        self.assertIn("chunk", channel.json_body)
-        self.assertEqual(channel.json_body["chunk"], [])
-
-        # The relations should not appear in the bundled aggregation.
-        channel = self.make_request(
-            "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/event/{self.parent_id}",
-            access_token=self.user_token,
-        )
-        self.assertEquals(200, channel.code, channel.json_body)
-        self.assertNotIn("m.relations", channel.json_body["unsigned"])
+        event_ids, relations = self._make_relation_requests()
+        self.assertEqual(event_ids, [])
+        self.assertEqual(relations, {})
 
         # Check that aggregations returns zero
         channel = self.make_request(

From 7862465fe6034000dbe6ef30ea096f8044ce9710 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Mar 2022 12:37:00 -0500
Subject: [PATCH 03/12] De-duplicate code to request aggregations.

---
 tests/rest/client/test_relations.py | 33 ++++++++++++-----------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py
index 1dd47aa0e6c2..26d67f87fffd 100644
--- a/tests/rest/client/test_relations.py
+++ b/tests/rest/client/test_relations.py
@@ -1387,6 +1387,16 @@ def _make_relation_requests(self) -> Tuple[List[str], JsonDict]:
 
         return event_ids, bundled_relations
 
+    def _get_aggregations(self) -> List[JsonDict]:
+        """Request /aggregations on the parent ID and includes the returned chunk."""
+        channel = self.make_request(
+            "GET",
+            f"/_matrix/client/unstable/rooms/{self.room}/aggregations/{self.parent_id}",
+            access_token=self.user_token,
+        )
+        self.assertEqual(200, channel.code, channel.json_body)
+        return channel.json_body["chunk"]
+
     def test_redact_relation_annotation(self) -> None:
         """
         Test that annotations of an event are properly handled after the
@@ -1417,17 +1427,8 @@ def test_redact_relation_annotation(self) -> None:
         )
 
         # The unredacted aggregation should still exist.
-        channel = self.make_request(
-            "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/aggregations/{self.parent_id}",
-            access_token=self.user_token,
-        )
-        self.assertEqual(200, channel.code, channel.json_body)
-
-        self.assertEqual(
-            channel.json_body,
-            {"chunk": [{"type": "m.reaction", "key": "a", "count": 1}]},
-        )
+        chunk = self._get_aggregations()
+        self.assertEqual(chunk, [{"type": "m.reaction", "key": "a", "count": 1}])
 
     def test_redact_relation_edit(self) -> None:
         """Test that edits of an event are redacted when the original event
@@ -1475,11 +1476,5 @@ def test_redact_parent(self) -> None:
         self.assertEqual(relations, {})
 
         # Check that aggregations returns zero
-        channel = self.make_request(
-            "GET",
-            f"/_matrix/client/unstable/rooms/{self.room}/aggregations/{self.parent_id}/m.annotation/m.reaction",
-            access_token=self.user_token,
-        )
-        self.assertEqual(200, channel.code, channel.json_body)
-        self.assertIn("chunk", channel.json_body)
-        self.assertEqual(channel.json_body["chunk"], [])
+        chunk = self._get_aggregations()
+        self.assertEqual(chunk, [])

From 99d9d8db41ddf45dfcc438bc2fe3be9d774f1b02 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Mar 2022 12:38:06 -0500
Subject: [PATCH 04/12] Add another assertion.

---
 tests/rest/client/test_relations.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py
index 26d67f87fffd..cf698c45a631 100644
--- a/tests/rest/client/test_relations.py
+++ b/tests/rest/client/test_relations.py
@@ -1448,8 +1448,9 @@ def test_redact_relation_edit(self) -> None:
         self.assertEqual(200, channel.code, channel.json_body)
 
         # Check the relation is returned
-        event_ids, _ = self._make_relation_requests()
+        event_ids, relations = self._make_relation_requests()
         self.assertEqual(len(event_ids), 1)
+        self.assertIn(RelationTypes.REPLACE, relations)
 
         # Redact the original event
         self._redact(self.parent_id)

From 5495b3408a340e8dbe7b871928b60942b0ef1121 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Mar 2022 12:51:50 -0500
Subject: [PATCH 05/12] Fix cache invalidation of redacted relations.

---
 synapse/storage/databases/main/events.py    | 17 +++++++++++++--
 synapse/storage/databases/main/relations.py |  8 +++++++
 tests/rest/client/test_relations.py         | 23 ++++++++++++++++++++-
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py
index ca2a9ba9d116..3b7926419faa 100644
--- a/synapse/storage/databases/main/events.py
+++ b/synapse/storage/databases/main/events.py
@@ -1943,15 +1943,28 @@ def _handle_batch_event(self, txn: LoggingTransaction, event: EventBase):
 
         txn.execute(sql, (batch_id,))
 
-    def _handle_redaction(self, txn, redacted_event_id):
+    def _handle_redaction(
+        self, txn: LoggingTransaction, redacted_event_id: str
+    ) -> None:
         """Handles receiving a redaction and checking whether we need to remove
         any redacted relations from the database.
 
         Args:
             txn
-            redacted_event_id (str): The event that was redacted.
+            redacted_event_id: The event that was redacted.
         """
 
+        # Fetch current relations.
+        related_events = self.db_pool.simple_select_onecol_txn(
+            txn,
+            table="event_relations",
+            keyvalues={"event_id": redacted_event_id},
+            retcol="relates_to_id",
+        )
+        # Any relation information for the related events must be cleared.
+        for related_event in related_events:
+            txn.call_after(self.store._invalidate_relations_for_event, related_event)
+
         self.db_pool.simple_delete_txn(
             txn, table="event_relations", keyvalues={"event_id": redacted_event_id}
         )
diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py
index 36aa1092f602..3a7165ad168f 100644
--- a/synapse/storage/databases/main/relations.py
+++ b/synapse/storage/databases/main/relations.py
@@ -91,6 +91,14 @@ def __init__(
 
         self._msc3440_enabled = hs.config.experimental.msc3440_enabled
 
+    def _invalidate_relations_for_event(self, event_id: str) -> None:
+        """Invalidate the relation caches for an event."""
+        self.get_relations_for_event.invalidate((event_id,))
+        self.get_aggregation_groups_for_event.invalidate((event_id,))
+        self.get_applicable_edit.invalidate((event_id,))
+        self.get_thread_summary.invalidate((event_id,))
+        self.get_thread_participated.invalidate((event_id,))
+
     @cached(tree=True)
     async def get_relations_for_event(
         self,
diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py
index cf698c45a631..d4d69c7c4616 100644
--- a/tests/rest/client/test_relations.py
+++ b/tests/rest/client/test_relations.py
@@ -1415,6 +1415,18 @@ def test_redact_relation_annotation(self) -> None:
         self.assertEqual(200, channel.code, channel.json_body)
         unredacted_event_id = channel.json_body["event_id"]
 
+        # Both relations should exist.
+        event_ids, relations = self._make_relation_requests()
+        self.assertCountEqual(event_ids, [to_redact_event_id, unredacted_event_id])
+        self.assertEquals(
+            relations["m.annotation"],
+            {"chunk": [{"type": "m.reaction", "key": "a", "count": 2}]},
+        )
+
+        # Both relations appear in the aggregation.
+        chunk = self._get_aggregations()
+        self.assertEqual(chunk, [{"type": "m.reaction", "key": "a", "count": 2}])
+
         # Redact one of the reactions.
         self._redact(to_redact_event_id)
 
@@ -1468,6 +1480,15 @@ def test_redact_parent(self) -> None:
         channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", key="👍")
         self.assertEqual(200, channel.code, channel.json_body)
 
+        # The relations should exist.
+        event_ids, relations = self._make_relation_requests()
+        self.assertEqual(len(event_ids), 1)
+        self.assertIn(RelationTypes.ANNOTATION, relations)
+
+        # The aggregation should exist.
+        chunk = self._get_aggregations()
+        self.assertEqual(chunk, [{"type": "m.reaction", "key": "👍", "count": 1}])
+
         # Redact the original event.
         self._redact(self.parent_id)
 
@@ -1476,6 +1497,6 @@ def test_redact_parent(self) -> None:
         self.assertEqual(event_ids, [])
         self.assertEqual(relations, {})
 
-        # Check that aggregations returns zero
+        # There's nothing to aggregate.
         chunk = self._get_aggregations()
         self.assertEqual(chunk, [])

From 5640989a85363e145cae0ec59a8550215e17b659 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Mar 2022 12:55:30 -0500
Subject: [PATCH 06/12] Rename a method for clarity.

---
 synapse/storage/databases/main/cache.py  |  4 +---
 synapse/storage/databases/main/events.py | 11 ++++++-----
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/synapse/storage/databases/main/cache.py b/synapse/storage/databases/main/cache.py
index c428dd5596af..8f2aa4989adc 100644
--- a/synapse/storage/databases/main/cache.py
+++ b/synapse/storage/databases/main/cache.py
@@ -197,9 +197,7 @@ def _invalidate_caches_for_event(
             self.get_invited_rooms_for_local_user.invalidate((state_key,))
 
         if relates_to:
-            self.get_relations_for_event.invalidate((relates_to,))
-            self.get_aggregation_groups_for_event.invalidate((relates_to,))
-            self.get_applicable_edit.invalidate((relates_to,))
+            self._invalidate_relations_for_event(relates_to)
 
     async def invalidate_cache_and_stream(self, cache_name: str, keys: Tuple[Any, ...]):
         """Invalidates the cache and adds it to the cache stream so slaves
diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py
index 3b7926419faa..2e142ad4d122 100644
--- a/synapse/storage/databases/main/events.py
+++ b/synapse/storage/databases/main/events.py
@@ -1518,7 +1518,7 @@ def _update_metadata_tables_txn(
                 )
 
                 # Remove from relations table.
-                self._handle_redaction(txn, event.redacts)
+                self._handle_redact_relations(txn, event.redacts)
 
         # Update the event_forward_extremities, event_backward_extremities and
         # event_edges tables.
@@ -1943,7 +1943,7 @@ def _handle_batch_event(self, txn: LoggingTransaction, event: EventBase):
 
         txn.execute(sql, (batch_id,))
 
-    def _handle_redaction(
+    def _handle_redact_relations(
         self, txn: LoggingTransaction, redacted_event_id: str
     ) -> None:
         """Handles receiving a redaction and checking whether we need to remove
@@ -1955,15 +1955,16 @@ def _handle_redaction(
         """
 
         # Fetch current relations.
-        related_events = self.db_pool.simple_select_onecol_txn(
+        relates_to = self.db_pool.simple_select_one_onecol_txn(
             txn,
             table="event_relations",
             keyvalues={"event_id": redacted_event_id},
             retcol="relates_to_id",
+            allow_none=True,
         )
         # Any relation information for the related events must be cleared.
-        for related_event in related_events:
-            txn.call_after(self.store._invalidate_relations_for_event, related_event)
+        if relates_to is not None:
+            txn.call_after(self.store._invalidate_relations_for_event, relates_to)
 
         self.db_pool.simple_delete_txn(
             txn, table="event_relations", keyvalues={"event_id": redacted_event_id}

From 9ae641524ca900f1263060e41e10ee0d76c76a99 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Mar 2022 13:45:07 -0500
Subject: [PATCH 07/12] Newsfragment

---
 changelog.d/12113.bugfix | 1 +
 changelog.d/12113.misc   | 1 -
 changelog.d/12121.bugfix | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)
 create mode 100644 changelog.d/12113.bugfix
 delete mode 100644 changelog.d/12113.misc
 create mode 100644 changelog.d/12121.bugfix

diff --git a/changelog.d/12113.bugfix b/changelog.d/12113.bugfix
new file mode 100644
index 000000000000..df9b0dc413dd
--- /dev/null
+++ b/changelog.d/12113.bugfix
@@ -0,0 +1 @@
+Fix a long-standing bug when redacting events with relations.
diff --git a/changelog.d/12113.misc b/changelog.d/12113.misc
deleted file mode 100644
index 102e064053c2..000000000000
--- a/changelog.d/12113.misc
+++ /dev/null
@@ -1 +0,0 @@
-Refactor the tests for event relations.
diff --git a/changelog.d/12121.bugfix b/changelog.d/12121.bugfix
new file mode 100644
index 000000000000..df9b0dc413dd
--- /dev/null
+++ b/changelog.d/12121.bugfix
@@ -0,0 +1 @@
+Fix a long-standing bug when redacting events with relations.

From 8907261fcaead3ad4730108aaf8982d7ae0f1ed0 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Tue, 1 Mar 2022 10:02:00 -0500
Subject: [PATCH 08/12] Add tests for thread relations.

---
 tests/rest/client/test_relations.py | 63 ++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py
index d4d69c7c4616..b77d65771d55 100644
--- a/tests/rest/client/test_relations.py
+++ b/tests/rest/client/test_relations.py
@@ -1442,7 +1442,68 @@ def test_redact_relation_annotation(self) -> None:
         chunk = self._get_aggregations()
         self.assertEqual(chunk, [{"type": "m.reaction", "key": "a", "count": 1}])
 
-    def test_redact_relation_edit(self) -> None:
+    @unittest.override_config({"experimental_features": {"msc3440_enabled": True}})
+    def test_redact_relation_thread(self) -> None:
+        """
+        Test that thread replies are properly handled after the thread reply redacted.
+
+        The redacted event should not be included in bundled aggregations or
+        the response to relations.
+        """
+        channel = self._send_relation(
+            RelationTypes.THREAD,
+            EventTypes.Message,
+            content={"body": "reply 1", "msgtype": "m.text"},
+        )
+        self.assertEqual(200, channel.code, channel.json_body)
+        unredacted_event_id = channel.json_body["event_id"]
+
+        # Note that the *last* event in the thread is redacted, as that gets
+        # included in the bundled aggregation.
+        channel = self._send_relation(
+            RelationTypes.THREAD,
+            EventTypes.Message,
+            content={"body": "reply 2", "msgtype": "m.text"},
+        )
+        self.assertEqual(200, channel.code, channel.json_body)
+        to_redact_event_id = channel.json_body["event_id"]
+
+        # Both relations exist.
+        event_ids, relations = self._make_relation_requests()
+        self.assertEquals(event_ids, [to_redact_event_id, unredacted_event_id])
+        self.assertDictContainsSubset(
+            {
+                "count": 2,
+                "current_user_participated": True,
+            },
+            relations[RelationTypes.THREAD],
+        )
+        # And the latest event returned is the event that will be redacted.
+        self.assertEqual(
+            relations[RelationTypes.THREAD]["latest_event"]["event_id"],
+            to_redact_event_id,
+        )
+
+        # Redact one of the reactions.
+        self._redact(to_redact_event_id)
+
+        # The unredacted relation should still exist.
+        event_ids, relations = self._make_relation_requests()
+        self.assertEquals(event_ids, [unredacted_event_id])
+        self.assertDictContainsSubset(
+            {
+                "count": 1,
+                "current_user_participated": True,
+            },
+            relations[RelationTypes.THREAD],
+        )
+        # And the latest event is now the unredacted event.
+        self.assertEqual(
+            relations[RelationTypes.THREAD]["latest_event"]["event_id"],
+            unredacted_event_id,
+        )
+
+    def test_redact_parent_edit(self) -> None:
         """Test that edits of an event are redacted when the original event
         is redacted.
         """

From b0d2a598487423914d9512648b1426e6e1ff37d6 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Wed, 2 Mar 2022 08:31:16 -0500
Subject: [PATCH 09/12] Rename a test for consistency.

---
 tests/rest/client/test_relations.py | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/tests/rest/client/test_relations.py b/tests/rest/client/test_relations.py
index b77d65771d55..3478a2905d96 100644
--- a/tests/rest/client/test_relations.py
+++ b/tests/rest/client/test_relations.py
@@ -1346,7 +1346,21 @@ def test_background_update(self) -> None:
 
 
 class RelationRedactionTestCase(BaseRelationsTestCase):
-    """Test the behaviour of relations when the parent or child event is redacted."""
+    """
+    Test the behaviour of relations when the parent or child event is redacted.
+
+    The behaviour of each relation type is subtly different which causes the tests
+    to be a bit repetitive, they follow a naming scheme of:
+
+        test_redact_(relation|parent)_{relation_type}
+
+    The first bit of "relation" means that the event with the relation defined
+    on it (the child event) is to be redacted. A "parent" means that the target
+    of the relation (the parent event) is to be redacted.
+
+    The relation_type describes which type of relation is under test (i.e. it is
+    related to the value of rel_type in the event content).
+    """
 
     def _redact(self, event_id: str) -> None:
         channel = self.make_request(
@@ -1533,7 +1547,7 @@ def test_redact_parent_edit(self) -> None:
         self.assertEqual(len(event_ids), 0)
         self.assertEqual(relations, {})
 
-    def test_redact_parent(self) -> None:
+    def test_redact_parent_annotation(self) -> None:
         """Test that annotations of an event are redacted when the original event
         is redacted.
         """

From e9100e290080458ebd0c93d0a9d59f07cee75574 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Fri, 4 Mar 2022 07:44:26 -0500
Subject: [PATCH 10/12] Properly redact caches.

---
 synapse/storage/databases/main/cache.py     |  6 +++++-
 synapse/storage/databases/main/events.py    | 16 +++++++++++++++-
 synapse/storage/databases/main/relations.py |  8 --------
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/synapse/storage/databases/main/cache.py b/synapse/storage/databases/main/cache.py
index 8f2aa4989adc..36af47e4781a 100644
--- a/synapse/storage/databases/main/cache.py
+++ b/synapse/storage/databases/main/cache.py
@@ -197,7 +197,11 @@ def _invalidate_caches_for_event(
             self.get_invited_rooms_for_local_user.invalidate((state_key,))
 
         if relates_to:
-            self._invalidate_relations_for_event(relates_to)
+            self.get_relations_for_event.invalidate((event_id,))
+            self.get_aggregation_groups_for_event.invalidate((event_id,))
+            self.get_applicable_edit.invalidate((event_id,))
+            self.get_thread_summary.invalidate((event_id,))
+            self.get_thread_participated.invalidate((event_id,))
 
     async def invalidate_cache_and_stream(self, cache_name: str, keys: Tuple[Any, ...]):
         """Invalidates the cache and adds it to the cache stream so slaves
diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py
index 2e142ad4d122..1b741dff770a 100644
--- a/synapse/storage/databases/main/events.py
+++ b/synapse/storage/databases/main/events.py
@@ -1964,7 +1964,21 @@ def _handle_redact_relations(
         )
         # Any relation information for the related events must be cleared.
         if relates_to is not None:
-            txn.call_after(self.store._invalidate_relations_for_event, relates_to)
+            self.store._invalidate_cache_and_stream(
+                txn, self.store.get_relations_for_event, (relates_to,)
+            )
+            self.store._invalidate_cache_and_stream(
+                txn, self.store.get_aggregation_groups_for_event, (relates_to,)
+            )
+            self.store._invalidate_cache_and_stream(
+                txn, self.store.get_applicable_edit, (relates_to,)
+            )
+            self.store._invalidate_cache_and_stream(
+                txn, self.store.get_thread_summary, (relates_to,)
+            )
+            self.store._invalidate_cache_and_stream(
+                txn, self.store.get_thread_participated, (relates_to,)
+            )
 
         self.db_pool.simple_delete_txn(
             txn, table="event_relations", keyvalues={"event_id": redacted_event_id}
diff --git a/synapse/storage/databases/main/relations.py b/synapse/storage/databases/main/relations.py
index 3a7165ad168f..36aa1092f602 100644
--- a/synapse/storage/databases/main/relations.py
+++ b/synapse/storage/databases/main/relations.py
@@ -91,14 +91,6 @@ def __init__(
 
         self._msc3440_enabled = hs.config.experimental.msc3440_enabled
 
-    def _invalidate_relations_for_event(self, event_id: str) -> None:
-        """Invalidate the relation caches for an event."""
-        self.get_relations_for_event.invalidate((event_id,))
-        self.get_aggregation_groups_for_event.invalidate((event_id,))
-        self.get_applicable_edit.invalidate((event_id,))
-        self.get_thread_summary.invalidate((event_id,))
-        self.get_thread_participated.invalidate((event_id,))
-
     @cached(tree=True)
     async def get_relations_for_event(
         self,

From 8b16a08486fb5a543677892ba5ab109eef916976 Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Fri, 4 Mar 2022 07:48:54 -0500
Subject: [PATCH 11/12] Fix copy & paste error.

---
 synapse/storage/databases/main/cache.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/synapse/storage/databases/main/cache.py b/synapse/storage/databases/main/cache.py
index 36af47e4781a..abd54c7dc703 100644
--- a/synapse/storage/databases/main/cache.py
+++ b/synapse/storage/databases/main/cache.py
@@ -197,11 +197,11 @@ def _invalidate_caches_for_event(
             self.get_invited_rooms_for_local_user.invalidate((state_key,))
 
         if relates_to:
-            self.get_relations_for_event.invalidate((event_id,))
-            self.get_aggregation_groups_for_event.invalidate((event_id,))
-            self.get_applicable_edit.invalidate((event_id,))
-            self.get_thread_summary.invalidate((event_id,))
-            self.get_thread_participated.invalidate((event_id,))
+            self.get_relations_for_event.invalidate((relates_to,))
+            self.get_aggregation_groups_for_event.invalidate((relates_to,))
+            self.get_applicable_edit.invalidate((relates_to,))
+            self.get_thread_summary.invalidate((relates_to,))
+            self.get_thread_participated.invalidate((relates_to,))
 
     async def invalidate_cache_and_stream(self, cache_name: str, keys: Tuple[Any, ...]):
         """Invalidates the cache and adds it to the cache stream so slaves

From 98a8e157d322e9c95eec516ddde60c752ceb892f Mon Sep 17 00:00:00 2001
From: Patrick Cloke <patrickc@matrix.org>
Date: Mon, 7 Mar 2022 08:32:26 -0500
Subject: [PATCH 12/12] Rename variable & clarify comments.

---
 synapse/storage/databases/main/events.py | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py
index 1b741dff770a..1dc83aa5e3a6 100644
--- a/synapse/storage/databases/main/events.py
+++ b/synapse/storage/databases/main/events.py
@@ -1946,38 +1946,38 @@ def _handle_batch_event(self, txn: LoggingTransaction, event: EventBase):
     def _handle_redact_relations(
         self, txn: LoggingTransaction, redacted_event_id: str
     ) -> None:
-        """Handles receiving a redaction and checking whether we need to remove
-        any redacted relations from the database.
+        """Handles receiving a redaction and checking whether the redacted event
+        has any relations which must be removed from the database.
 
         Args:
             txn
             redacted_event_id: The event that was redacted.
         """
 
-        # Fetch current relations.
-        relates_to = self.db_pool.simple_select_one_onecol_txn(
+        # Fetch the current relation of the event being redacted.
+        redacted_relates_to = self.db_pool.simple_select_one_onecol_txn(
             txn,
             table="event_relations",
             keyvalues={"event_id": redacted_event_id},
             retcol="relates_to_id",
             allow_none=True,
         )
-        # Any relation information for the related events must be cleared.
-        if relates_to is not None:
+        # Any relation information for the related event must be cleared.
+        if redacted_relates_to is not None:
             self.store._invalidate_cache_and_stream(
-                txn, self.store.get_relations_for_event, (relates_to,)
+                txn, self.store.get_relations_for_event, (redacted_relates_to,)
             )
             self.store._invalidate_cache_and_stream(
-                txn, self.store.get_aggregation_groups_for_event, (relates_to,)
+                txn, self.store.get_aggregation_groups_for_event, (redacted_relates_to,)
             )
             self.store._invalidate_cache_and_stream(
-                txn, self.store.get_applicable_edit, (relates_to,)
+                txn, self.store.get_applicable_edit, (redacted_relates_to,)
             )
             self.store._invalidate_cache_and_stream(
-                txn, self.store.get_thread_summary, (relates_to,)
+                txn, self.store.get_thread_summary, (redacted_relates_to,)
             )
             self.store._invalidate_cache_and_stream(
-                txn, self.store.get_thread_participated, (relates_to,)
+                txn, self.store.get_thread_participated, (redacted_relates_to,)
             )
 
         self.db_pool.simple_delete_txn(