From 2b19e6920cee036b6e80b48d10e6b868eeb0ff08 Mon Sep 17 00:00:00 2001 From: Incognito Beeper Date: Fri, 3 Mar 2023 13:44:57 +0000 Subject: [PATCH 1/8] Stop sending receipts to large rooms --- synapse/api/constants.py | 3 +++ synapse/handlers/receipts.py | 10 ++++++++- synapse/types/__init__.py | 16 ++++++++++++++ tests/rest/client/test_sync.py | 40 ++++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 1 deletion(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 5a943b02f34a..9f9819b16108 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -277,6 +277,9 @@ class ReceiptTypes: BEEPER_INBOX_DONE: Final = "com.beeper.inbox.done" +RECEIPTS_MAX_ROOM_SIZE = 100 + + class PublicRoomsFilterFields: """Fields in the search filter for `/publicRooms` that we understand. diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index c9be10a574f5..cc959e497d5d 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -14,7 +14,7 @@ import logging from typing import TYPE_CHECKING, Iterable, List, Optional, Tuple -from synapse.api.constants import EduTypes, ReceiptTypes +from synapse.api.constants import RECEIPTS_MAX_ROOM_SIZE, EduTypes, ReceiptTypes from synapse.appservice import ApplicationService from synapse.streams import EventSource from synapse.types import ( @@ -116,6 +116,14 @@ async def _handle_new_receipts(self, receipts: List[ReadReceipt]) -> bool: min_batch_id: Optional[int] = None max_batch_id: Optional[int] = None + # Beeper: we don't want to send read receipts to large rooms, + # so we convert messages to private, that are over RECEIPT_MAX_ROOM_SIZE. + for i, r in enumerate(receipts): + if r.receipt_type != ReceiptTypes.READ_PRIVATE: + num_users = await self.store.get_number_joined_users_in_room(r.room_id) + if num_users > RECEIPTS_MAX_ROOM_SIZE: + receipts[i] = r.make_private_copy() + for receipt in receipts: res = await self.store.insert_receipt( receipt.room_id, diff --git a/synapse/types/__init__.py b/synapse/types/__init__.py index f82d1cfc298b..a4f78b4ddecc 100644 --- a/synapse/types/__init__.py +++ b/synapse/types/__init__.py @@ -12,6 +12,8 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from __future__ import annotations + import abc import re import string @@ -52,6 +54,7 @@ IReactorTime, ) +from synapse.api.constants import ReceiptTypes from synapse.api.errors import Codes, SynapseError from synapse.util.cancellation import cancellable from synapse.util.stringutils import parse_and_validate_server_name @@ -853,6 +856,19 @@ class ReadReceipt: thread_id: Optional[str] data: JsonDict + # Beeper: we don't want to alter the frozen attr, but + # do occasionally need to make a private copy to + # avoid traffic to large rooms. + def make_private_copy(self) -> ReadReceipt: + return ReadReceipt( + room_id=self.room_id, + user_id=self.user_id, + event_ids=self.event_ids, + thread_id=self.thread_id, + data=self.data, + receipt_type=ReceiptTypes.READ_PRIVATE, + ) + @attr.s(slots=True, frozen=True, auto_attribs=True) class DeviceListUpdates: diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 2cb624ce7b57..473a98624db8 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -22,6 +22,7 @@ import synapse.rest.admin from synapse.api.constants import ( + RECEIPTS_MAX_ROOM_SIZE, EduTypes, EventContentFields, EventTypes, @@ -382,6 +383,7 @@ class ReadReceiptsTestCase(unittest.HomeserverTestCase): servlets = [ synapse.rest.admin.register_servlets, login.register_servlets, + read_marker.register_servlets, receipts.register_servlets, room.register_servlets, sync.register_servlets, @@ -499,6 +501,35 @@ def test_read_receipt_with_empty_body_is_rejected(self) -> None: self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST) self.assertEqual(channel.json_body["errcode"], "M_NOT_JSON", channel.json_body) + def test_read_receipt_not_sent_to_large_rooms(self) -> None: + # Beeper: we don't send read receipts on rooms with more + # users than # RECEIPTS_MAX_ROOM_SIZE + for i in range(RECEIPTS_MAX_ROOM_SIZE): + user = self.register_user(f"user_{i}", f"secure_password_{i}") + tok = self.login(f"user_{i}", f"secure_password_{i}") + self.helper.join(room=self.room_id, user=user, tok=tok) + + res = self.helper.send( + self.room_id, body="woah, this is a big room!", tok=self.tok + ) + + for receipt_type in ( + ReceiptTypes.FULLY_READ, + ReceiptTypes.READ_PRIVATE, + ReceiptTypes.READ, + ): + # Send a read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/receipt/{receipt_type}/{res['event_id']}", + {}, + access_token=self.tok2, + ) + self.assertEqual(channel.code, 200) + + # Test that we didn't get a read receipt. + self.assertIsNone(self._get_read_receipt()) + def _get_read_receipt(self) -> Optional[JsonDict]: """Syncs and returns the read receipt.""" @@ -728,6 +759,15 @@ def test_unread_counts(self) -> None: self.assertEqual(channel.code, 200, channel.json_body) self._check_unread_count(0) + def test_large_rooms_dont_alter_unread_count_behaviour(self) -> None: + # Beeper: create a lot of users and join them to the room + for i in range(RECEIPTS_MAX_ROOM_SIZE): + user = self.register_user(f"user_{i}", f"secure_password_{i}") + tok = self.login(f"user_{i}", f"secure_password_{i}") + self.helper.join(self.room_id, user=user, tok=tok) + + self.test_unread_counts() + # We test for all three receipt types that influence notification counts @parameterized.expand( [ From 288302eff0117ed61edf3e8562ea6475ae9123b7 Mon Sep 17 00:00:00 2001 From: John Newman Date: Mon, 6 Mar 2023 21:58:37 +0000 Subject: [PATCH 2/8] update read_marker so receipts aren't sent to large rooms --- synapse/rest/client/read_marker.py | 11 ++++++- tests/rest/client/test_sync.py | 47 ++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/synapse/rest/client/read_marker.py b/synapse/rest/client/read_marker.py index 914766b7b0c2..4f6bff6409d8 100644 --- a/synapse/rest/client/read_marker.py +++ b/synapse/rest/client/read_marker.py @@ -15,7 +15,7 @@ import logging from typing import TYPE_CHECKING, Tuple -from synapse.api.constants import ReceiptTypes +from synapse.api.constants import RECEIPTS_MAX_ROOM_SIZE, ReceiptTypes from synapse.http.server import HttpServer from synapse.http.servlet import RestServlet, parse_json_object_from_request from synapse.http.site import SynapseRequest @@ -38,6 +38,7 @@ def __init__(self, hs: "HomeServer"): self.config = hs.config self.receipts_handler = hs.get_receipts_handler() self.read_marker_handler = hs.get_read_marker_handler() + self.store = hs.get_datastores().main self.presence_handler = hs.get_presence_handler() self._known_receipt_types = { @@ -55,6 +56,14 @@ async def on_POST( body = parse_json_object_from_request(request) + # Beeper: we don't want to send read receipts to large rooms, + # so we convert messages to private, that are over RECEIPT_MAX_ROOM_SIZE. + if ReceiptTypes.READ in body: + num_users = await self.store.get_number_joined_users_in_room(room_id) + if num_users > RECEIPTS_MAX_ROOM_SIZE: + body[ReceiptTypes.READ_PRIVATE] = body[ReceiptTypes.READ] + del body[ReceiptTypes.READ] + unrecognized_types = set(body.keys()) - self._known_receipt_types if unrecognized_types: # It's fine if there are unrecognized receipt types, but let's log diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 473a98624db8..bdb38e2037c3 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -530,6 +530,53 @@ def test_read_receipt_not_sent_to_large_rooms(self) -> None: # Test that we didn't get a read receipt. self.assertIsNone(self._get_read_receipt()) + def test_read_marker_doesnt_send_receipt_to_large_rooms(self) -> None: + # Beeper: we don't send read receipts on rooms with over 100 users + # add another 100 users to the room + for i in range(RECEIPTS_MAX_ROOM_SIZE): + user = self.register_user(f"user_{i}", f"secure_password_{i}") + tok = self.login(f"user_{i}", f"secure_password_{i}") + self.helper.join(room=self.room_id, user=user, tok=tok) + + res = self.helper.send( + self.room_id, body="woah, this is a big room!", tok=self.tok + ) + + # Send a read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/read_markers", + { + ReceiptTypes.FULLY_READ: res["event_id"], + ReceiptTypes.READ: res["event_id"], + }, + access_token=self.tok2, + ) + + self.assertEqual(channel.code, 200, channel.json_body) + # Test that we didn't get a read receipt. + self.assertIsNone(self._get_read_receipt()) + + def test_read_marker_does_send_receipt_to_small_rooms(self) -> None: + res = self.helper.send( + self.room_id, body="woah, this is room is tiny!", tok=self.tok + ) + + # Send a read receipt + channel = self.make_request( + "POST", + f"/rooms/{self.room_id}/read_markers", + { + ReceiptTypes.FULLY_READ: res["event_id"], + ReceiptTypes.READ: res["event_id"], + }, + access_token=self.tok2, + ) + + self.assertEqual(channel.code, 200, channel.json_body) + # Test that we didn't get a read receipt. + self.assertIsNotNone(self._get_read_receipt()) + def _get_read_receipt(self) -> Optional[JsonDict]: """Syncs and returns the read receipt.""" From a6f297f9ad3ed6cba5e68ea6433864477657e500 Mon Sep 17 00:00:00 2001 From: John Newman Date: Fri, 3 Mar 2023 13:50:24 +0000 Subject: [PATCH 3/8] Up the cache it will be hit often now --- synapse/storage/databases/main/roommember.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 3abc4dcfef1a..47ca0278375f 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -349,7 +349,7 @@ def _get_room_summary_txn( "get_room_summary", _get_room_summary_txn ) - @cached() + @cached(max_entries=100000) async def get_number_joined_users_in_room(self, room_id: str) -> int: return await self.db_pool.simple_select_one_onecol( table="current_state_events", From f74f4bfdaf004875c52f8dc42e6386eb67618f4c Mon Sep 17 00:00:00 2001 From: John Newman Date: Sun, 5 Mar 2023 23:50:35 +0000 Subject: [PATCH 4/8] Remove BEEPER_INBOX_DONE, no longer used. --- synapse/api/constants.py | 1 - synapse/rest/client/notifications.py | 1 - synapse/rest/client/receipts.py | 1 - 3 files changed, 3 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 9f9819b16108..c2006437de03 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -274,7 +274,6 @@ class ReceiptTypes: READ: Final = "m.read" READ_PRIVATE: Final = "m.read.private" FULLY_READ: Final = "m.fully_read" - BEEPER_INBOX_DONE: Final = "com.beeper.inbox.done" RECEIPTS_MAX_ROOM_SIZE = 100 diff --git a/synapse/rest/client/notifications.py b/synapse/rest/client/notifications.py index 4bb44c24dba1..61268e3af1e2 100644 --- a/synapse/rest/client/notifications.py +++ b/synapse/rest/client/notifications.py @@ -61,7 +61,6 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: user_id, [ ReceiptTypes.READ, - ReceiptTypes.BEEPER_INBOX_DONE, ReceiptTypes.READ_PRIVATE, ], ) diff --git a/synapse/rest/client/receipts.py b/synapse/rest/client/receipts.py index 8aa8ab1b79df..3ab42b53d377 100644 --- a/synapse/rest/client/receipts.py +++ b/synapse/rest/client/receipts.py @@ -49,7 +49,6 @@ def __init__(self, hs: "HomeServer"): ReceiptTypes.READ, ReceiptTypes.READ_PRIVATE, ReceiptTypes.FULLY_READ, - ReceiptTypes.BEEPER_INBOX_DONE, } async def on_POST( From fbcd582f6e988b7ac6176807ab2b70e91889b8f2 Mon Sep 17 00:00:00 2001 From: John Newman Date: Tue, 7 Mar 2023 21:48:41 +0000 Subject: [PATCH 5/8] check size of each room just once --- synapse/handlers/receipts.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index cc959e497d5d..e63b160b6f12 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -118,11 +118,19 @@ async def _handle_new_receipts(self, receipts: List[ReadReceipt]) -> bool: # Beeper: we don't want to send read receipts to large rooms, # so we convert messages to private, that are over RECEIPT_MAX_ROOM_SIZE. + room_ids_to_check = { + r.room_id for r in receipts if r.receipt_type != ReceiptTypes.READ_PRIVATE + } + + large_rooms = [] + for room_id in room_ids_to_check: + num_users = await self.store.get_number_joined_users_in_room(room_id) + if num_users > RECEIPTS_MAX_ROOM_SIZE: + large_rooms.append(room_id) + for i, r in enumerate(receipts): - if r.receipt_type != ReceiptTypes.READ_PRIVATE: - num_users = await self.store.get_number_joined_users_in_room(r.room_id) - if num_users > RECEIPTS_MAX_ROOM_SIZE: - receipts[i] = r.make_private_copy() + if r.room_id in large_rooms: + receipts[i] = r.make_private_copy() for receipt in receipts: res = await self.store.insert_receipt( From f6eaf803e909c5e81200cb89d644cd3de04a692b Mon Sep 17 00:00:00 2001 From: John Newman Date: Mon, 13 Mar 2023 00:33:17 +0000 Subject: [PATCH 6/8] check room sizes in parallel --- synapse/handlers/receipts.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index e63b160b6f12..96b5d6db9b02 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -16,6 +16,7 @@ from synapse.api.constants import RECEIPTS_MAX_ROOM_SIZE, EduTypes, ReceiptTypes from synapse.appservice import ApplicationService +from synapse.util.async_helpers import yieldable_gather_results from synapse.streams import EventSource from synapse.types import ( JsonDict, @@ -122,10 +123,13 @@ async def _handle_new_receipts(self, receipts: List[ReadReceipt]) -> bool: r.room_id for r in receipts if r.receipt_type != ReceiptTypes.READ_PRIVATE } + room_sizes = await yieldable_gather_results( + lambda room_id: self.store.get_number_joined_users_in_room(room_id), + (room_id for room_id in room_ids_to_check) + ) large_rooms = [] - for room_id in room_ids_to_check: - num_users = await self.store.get_number_joined_users_in_room(room_id) - if num_users > RECEIPTS_MAX_ROOM_SIZE: + for room_id, room_size in zip(room_ids_to_check, room_sizes): + if room_size > RECEIPTS_MAX_ROOM_SIZE: large_rooms.append(room_id) for i, r in enumerate(receipts): From 8ba17a46d46325db8f4d9c5d43f2b37f731dc8d5 Mon Sep 17 00:00:00 2001 From: John Newman Date: Wed, 22 Mar 2023 17:28:58 +0000 Subject: [PATCH 7/8] correct import sort order --- synapse/handlers/receipts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index 96b5d6db9b02..d93e9440fbf2 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -16,7 +16,6 @@ from synapse.api.constants import RECEIPTS_MAX_ROOM_SIZE, EduTypes, ReceiptTypes from synapse.appservice import ApplicationService -from synapse.util.async_helpers import yieldable_gather_results from synapse.streams import EventSource from synapse.types import ( JsonDict, @@ -25,6 +24,7 @@ UserID, get_domain_from_id, ) +from synapse.util.async_helpers import yieldable_gather_results if TYPE_CHECKING: from synapse.server import HomeServer From 4583367f3bea38d8e8827eaad6ae81f40d336b2c Mon Sep 17 00:00:00 2001 From: John Newman Date: Wed, 22 Mar 2023 17:33:34 +0000 Subject: [PATCH 8/8] add that missing comma --- synapse/handlers/receipts.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/handlers/receipts.py b/synapse/handlers/receipts.py index d93e9440fbf2..3010b8af424f 100644 --- a/synapse/handlers/receipts.py +++ b/synapse/handlers/receipts.py @@ -125,7 +125,7 @@ async def _handle_new_receipts(self, receipts: List[ReadReceipt]) -> bool: room_sizes = await yieldable_gather_results( lambda room_id: self.store.get_number_joined_users_in_room(room_id), - (room_id for room_id in room_ids_to_check) + (room_id for room_id in room_ids_to_check), ) large_rooms = [] for room_id, room_size in zip(room_ids_to_check, room_sizes):