From 7bc58b9df47ca96b773687eb6a22a423d87f53f4 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 18:10:14 +0000 Subject: [PATCH 01/12] `FederationClient.backfill`: stop flagging events as outliers Events returned by `backfill` should not be flagged as outliers. Fixes: ``` AssertionError: null File "synapse/handlers/federation.py", line 313, in try_backfill dom, room_id, limit=100, extremities=extremities File "synapse/handlers/federation_event.py", line 517, in backfill await self._process_pulled_events(dest, events, backfilled=True) File "synapse/handlers/federation_event.py", line 642, in _process_pulled_events await self._process_pulled_event(origin, ev, backfilled=backfilled) File "synapse/handlers/federation_event.py", line 669, in _process_pulled_event assert not event.internal_metadata.is_outlier() ``` See https://sentry.matrix.org/sentry/synapse-matrixorg/issues/231992 Fixes #8894. --- changelog.d/11632.bugfix | 1 + synapse/federation/federation_client.py | 2 +- synapse/handlers/federation_event.py | 4 +++- 3 files changed, 5 insertions(+), 2 deletions(-) create mode 100644 changelog.d/11632.bugfix diff --git a/changelog.d/11632.bugfix b/changelog.d/11632.bugfix new file mode 100644 index 000000000000..c73d41652a87 --- /dev/null +++ b/changelog.d/11632.bugfix @@ -0,0 +1 @@ +Fix a bug introduced in Synapse 1.19.3 which could sometimes cause `AssertionError`s when backfilling rooms over federation. diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index fee1477ab684..7353c2b6b147 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -272,7 +272,7 @@ async def backfill( # Check signatures and hash of pdus, removing any from the list that fail checks pdus[:] = await self._check_sigs_and_hash_and_fetch( - dest, pdus, outlier=True, room_version=room_version + dest, pdus, room_version=room_version ) return pdus diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 9917613298c6..b4d9e41bd740 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -666,7 +666,9 @@ async def _process_pulled_event( logger.info("Processing pulled event %s", event) # these should not be outliers. - assert not event.internal_metadata.is_outlier() + assert ( + not event.internal_metadata.is_outlier() + ), "pulled event unexpectedly flagged as outlier" event_id = event.event_id From 44fc78996eb417b38eae930acef129d10e271355 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 17:45:17 +0000 Subject: [PATCH 02/12] `_auth_and_persist_outliers`: mark persisted events as outliers Mark any events that get persisted via `_auth_and_persist_outliers` as, well, outliers. Currently this will be a no-op as everything will already be flagged as an outlier, but I'm going to change that. --- synapse/handlers/federation_event.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index b4d9e41bd740..118e0eca7231 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1314,6 +1314,9 @@ def prep(event: EventBase) -> Optional[Tuple[EventBase, EventContext]]: return None auth.append(ae) + # we're not bothering about room state, so flag the event as an outlier. + event.internal_metadata.outlier = True + context = EventContext.for_outlier() try: validate_event_for_room_version(room_version_obj, event) From 893eb7b2de99aef1bee1fc8a9d5cbe723dccc187 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 17:53:15 +0000 Subject: [PATCH 03/12] `process_remote_join`: stop flagging as outlier The events are now flagged as outliers later on, by `_auth_and_persist_outliers`. --- synapse/handlers/federation_event.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 118e0eca7231..9c5dd91f8c58 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -421,9 +421,6 @@ async def process_remote_join( Raises: SynapseError if the response is in some way invalid. """ - for e in itertools.chain(auth_events, state): - e.internal_metadata.outlier = True - event_map = {e.event_id: e for e in itertools.chain(auth_events, state)} create_event = None From 49536a01b3983a2269b0f9fbfce1a3e512868dcc Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 21:03:27 +0000 Subject: [PATCH 04/12] `send_join`: remove `outlier=True` The events created here are returned in the result of `send_join` to `FederationHandler.do_invite_join`. From there they are passed into `FederationEventHandler.process_remote_join`, which passes them to `_auth_and_persist_outliers`... which sets the `outlier` flag. --- synapse/federation/federation_client.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 7353c2b6b147..557011db77f5 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -816,7 +816,6 @@ async def send_request(destination: str) -> SendJoinResult: valid_pdu = await self._check_sigs_and_hash_and_fetch_one( pdu=event, origin=destination, - outlier=True, room_version=room_version, ) @@ -864,7 +863,6 @@ async def _execute(pdu: EventBase) -> None: valid_pdu = await self._check_sigs_and_hash_and_fetch_one( pdu=pdu, origin=destination, - outlier=True, room_version=room_version, ) From 7a01a9caad72ac4678dc31c500b97a36b517b819 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 17:55:44 +0000 Subject: [PATCH 05/12] `get_event_auth`: remove `outlier=True` stop flagging the events returned by `get_event_auth` as outliers. This method is only called by `_get_remote_auth_chain_for_event`, which passes the results into `_auth_and_persist_outliers`, which will flag them as outliers. --- synapse/federation/federation_client.py | 7 ++----- tests/handlers/test_federation.py | 4 +--- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 557011db77f5..50f6cef010ef 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -541,13 +541,10 @@ async def get_event_auth( room_version = await self.store.get_room_version(room_id) - auth_chain = [ - event_from_pdu_json(p, room_version, outlier=True) - for p in res["auth_chain"] - ] + auth_chain = [event_from_pdu_json(p, room_version) for p in res["auth_chain"]] signed_auth = await self._check_sigs_and_hash_and_fetch( - destination, auth_chain, outlier=True, room_version=room_version + destination, auth_chain, room_version=room_version ) return signed_auth diff --git a/tests/handlers/test_federation.py b/tests/handlers/test_federation.py index e1557566e4bc..496b58172643 100644 --- a/tests/handlers/test_federation.py +++ b/tests/handlers/test_federation.py @@ -373,9 +373,7 @@ async def get_event_auth( destination: str, room_id: str, event_id: str ) -> List[EventBase]: return [ - event_from_pdu_json( - ae.get_pdu_json(), room_version=room_version, outlier=True - ) + event_from_pdu_json(ae.get_pdu_json(), room_version=room_version) for ae in auth_events ] From bca9f1195bdc78042b9e99b855d089c5dec0f760 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 18:00:55 +0000 Subject: [PATCH 06/12] `_get_remote_auth_chain_for_event`: remove `outlier=True` we pass all the events into `_auth_and_persist_outliers`, which will now flag the events as outliers. --- synapse/handlers/federation_event.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 9c5dd91f8c58..9ac780518159 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1191,7 +1191,6 @@ async def get_event(event_id: str) -> None: [destination], event_id, room_version, - outlier=True, ) if event is None: logger.warning( From 926cecb39e7346def52169742a5d8bfbccea06fe Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 18:31:43 +0000 Subject: [PATCH 07/12] `_check_sigs_and_hash_and_fetch`: remove unused `outlier` parameter This param is now never set to True, so we can remove it. --- synapse/federation/federation_client.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 50f6cef010ef..61f8e778ec8c 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -435,7 +435,6 @@ async def _check_sigs_and_hash_and_fetch( origin: str, pdus: Collection[EventBase], room_version: RoomVersion, - outlier: bool = False, ) -> List[EventBase]: """Takes a list of PDUs and checks the signatures and hashes of each one. If a PDU fails its signature check then we check if we have it in @@ -451,7 +450,6 @@ async def _check_sigs_and_hash_and_fetch( origin pdu room_version - outlier: Whether the events are outliers or not Returns: A list of PDUs that have valid signatures and hashes. @@ -466,7 +464,6 @@ async def _execute(pdu: EventBase) -> None: valid_pdu = await self._check_sigs_and_hash_and_fetch_one( pdu=pdu, origin=origin, - outlier=outlier, room_version=room_version, ) @@ -1230,7 +1227,7 @@ async def get_missing_events( ] signed_events = await self._check_sigs_and_hash_and_fetch( - destination, events, outlier=False, room_version=room_version + destination, events, room_version=room_version ) except HttpResponseException as e: if not e.code == 400: From 220c163e0d505258180a82fa477907765e566433 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 21:09:39 +0000 Subject: [PATCH 08/12] `_check_sigs_and_hash_and_fetch_one`: remove unused `outlier` param This is no longer set anywhere, so we can remove it. --- synapse/federation/federation_client.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 61f8e778ec8c..ed0588bd1ab8 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -479,7 +479,6 @@ async def _check_sigs_and_hash_and_fetch_one( pdu: EventBase, origin: str, room_version: RoomVersion, - outlier: bool = False, ) -> Optional[EventBase]: """Takes a PDU and checks its signatures and hashes. If the PDU fails its signature check then we check if we have it in the database and if @@ -491,9 +490,6 @@ async def _check_sigs_and_hash_and_fetch_one( origin pdu room_version - outlier: Whether the events are outliers or not - include_none: Whether to include None in the returned list - for events that have failed their checks Returns: The PDU (possibly redacted) if it has valid signatures and hashes. @@ -518,7 +514,6 @@ async def _check_sigs_and_hash_and_fetch_one( destinations=[pdu_origin], event_id=pdu.event_id, room_version=room_version, - outlier=outlier, timeout=10000, ) except SynapseError: From b82366d8986ae556d6a06586399274ae8f49ac0a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 21:11:29 +0000 Subject: [PATCH 09/12] `get_pdu`: remove unused `outlier` parameter ... and chase it down into `get_pdu_from_destination_raw`. --- synapse/federation/federation_client.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index ed0588bd1ab8..ee71eaf6ba91 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -282,7 +282,6 @@ async def get_pdu_from_destination_raw( destination: str, event_id: str, room_version: RoomVersion, - outlier: bool = False, timeout: Optional[int] = None, ) -> Optional[EventBase]: """Requests the PDU with given origin and ID from the remote home @@ -292,9 +291,6 @@ async def get_pdu_from_destination_raw( destination: Which homeserver to query event_id: event to fetch room_version: version of the room - outlier: Indicates whether the PDU is an `outlier`, i.e. if - it's from an arbitrary point in the context as opposed to part - of the current block of PDUs. Defaults to `False` timeout: How long to try (in ms) each destination for before moving to the next destination. None indicates no timeout. @@ -316,8 +312,7 @@ async def get_pdu_from_destination_raw( ) pdu_list: List[EventBase] = [ - event_from_pdu_json(p, room_version, outlier=outlier) - for p in transaction_data["pdus"] + event_from_pdu_json(p, room_version) for p in transaction_data["pdus"] ] if pdu_list and pdu_list[0]: @@ -334,7 +329,6 @@ async def get_pdu( destinations: Iterable[str], event_id: str, room_version: RoomVersion, - outlier: bool = False, timeout: Optional[int] = None, ) -> Optional[EventBase]: """Requests the PDU with given origin and ID from the remote home @@ -347,9 +341,6 @@ async def get_pdu( destinations: Which homeservers to query event_id: event to fetch room_version: version of the room - outlier: Indicates whether the PDU is an `outlier`, i.e. if - it's from an arbitrary point in the context as opposed to part - of the current block of PDUs. Defaults to `False` timeout: How long to try (in ms) each destination for before moving to the next destination. None indicates no timeout. @@ -377,7 +368,6 @@ async def get_pdu( destination=destination, event_id=event_id, room_version=room_version, - outlier=outlier, timeout=timeout, ) From 3f9d05f9ee43883921dc11f626b6d217a807ad80 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 21:15:08 +0000 Subject: [PATCH 10/12] `event_from_pdu_json`: remove redundant `outlier` param This is never set to `True`, so can be removed. --- synapse/federation/federation_base.py | 7 +------ synapse/federation/federation_client.py | 5 +---- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index f56344a3b94f..cdd18dc38cb7 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -220,15 +220,12 @@ def _is_invite_via_3pid(event: EventBase) -> bool: ) -def event_from_pdu_json( - pdu_json: JsonDict, room_version: RoomVersion, outlier: bool = False -) -> EventBase: +def event_from_pdu_json(pdu_json: JsonDict, room_version: RoomVersion) -> EventBase: """Construct an EventBase from an event json received over federation Args: pdu_json: pdu as received over federation room_version: The version of the room this event belongs to - outlier: True to mark this event as an outlier Raises: SynapseError: if the pdu is missing required fields or is otherwise @@ -252,6 +249,4 @@ def event_from_pdu_json( validate_canonicaljson(pdu_json) event = make_event_from_dict(pdu_json, room_version) - event.internal_metadata.outlier = outlier - return event diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index ee71eaf6ba91..6ea4edfc71f8 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -265,10 +265,7 @@ async def backfill( room_version = await self.store.get_room_version(room_id) - pdus = [ - event_from_pdu_json(p, room_version, outlier=False) - for p in transaction_data_pdus - ] + pdus = [event_from_pdu_json(p, room_version) for p in transaction_data_pdus] # Check signatures and hash of pdus, removing any from the list that fail checks pdus[:] = await self._check_sigs_and_hash_and_fetch( From 9564243669c7fbce9e3453b6c637fa7d28db0941 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 Dec 2021 21:22:09 +0000 Subject: [PATCH 11/12] changelog --- changelog.d/11634.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11634.misc diff --git a/changelog.d/11634.misc b/changelog.d/11634.misc new file mode 100644 index 000000000000..4069cbc2f483 --- /dev/null +++ b/changelog.d/11634.misc @@ -0,0 +1 @@ +Refactor the way that the `outlier` flag is set on events received over federation. From b6417bda778455109f3728826dd6971cf63923a6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 5 Jan 2022 11:57:42 +0000 Subject: [PATCH 12/12] update docstring --- synapse/handlers/federation_event.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/federation_event.py b/synapse/handlers/federation_event.py index 28170f95f8c3..11771f3c9c2b 100644 --- a/synapse/handlers/federation_event.py +++ b/synapse/handlers/federation_event.py @@ -1219,9 +1219,10 @@ async def _auth_and_persist_outliers( """Persist a batch of outlier events fetched from remote servers. We first sort the events to make sure that we process each event's auth_events - before the event itself, and then auth and persist them. + before the event itself. - Notifies about the events where appropriate. + We then mark the events as outliers, persist them to the database, and, where + appropriate (eg, an invite), awake the notifier. Params: room_id: the room that the events are meant to be in (though this has @@ -1272,7 +1273,8 @@ async def _auth_and_persist_outliers_inner( Persists a batch of events where we have (theoretically) already persisted all of their auth events. - Notifies about the events where appropriate. + Marks the events as outliers, auths them, persists them to the database, and, + where appropriate (eg, an invite), awakes the notifier. Params: origin: where the events came from