Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Exclude OOB memberships from the federation sender #12570

Merged
merged 3 commits into from
May 3, 2022
Merged

Exclude OOB memberships from the federation sender #12570

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
78dad8a
Exclude OOB memberships from the federation sender
richvdh Apr 27, 2022
947f651
Update docstring on `is_out_of_band_membership`
richvdh May 3, 2022
ca5b502
Wording updates for comments
richvdh May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/12570.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug introduced in Synapse 1.57 which could cause `Failed to calculate hosts in room` errors to be logged for outbound federation.
33 changes: 33 additions & 0 deletions synapse/federation/sender/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,39 @@ async def handle_event(event: EventBase) -> None:
if not is_mine and send_on_behalf_of is None:
return

# We also want to not send out-of-band membership events.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is out-of-band membership a spec-defined or Synapse-internal term? (Seems to be the latter.) Is there a reference for what they are somewhere?

The 3.5 cases below seem to enumerate them and explain why we don't want to send them out. It might be useful to have this list of 3.5 event types here (a more authoritative place?):

synapse/synapse/events/__init__.py

Lines 214 to 223 in c027bc0

def is_out_of_band_membership(self) -> bool:
"""Whether this is an out of band membership, like an invite or an invite
rejection. This is needed as those events are marked as outliers, but
they still need to be processed as if they're new events (e.g. updating
invite state in the database, relaying to clients, etc).
(Added in synapse 0.99.0, so may be unreliable for events received before that)
"""
return self._dict.get("out_of_band_membership", False)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is out-of-band membership a spec-defined or Synapse-internal term? (Seems to be the latter.) Is there a reference for what they are somewhere?

https://matrix-org.github.io/synapse/develop/development/room-dag-concepts.html#out-of-band-membership-events has some info on it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 3.5 cases below seem to enumerate them and explain why we don't want to send them out. It might be useful to have this list of 3.5 event types here (a more authoritative place?):

The 3.5 cases here are intended as a proof by enumeration that it's correct to drop these events in the federation sender, rather than acting as an authoritative list of what OOB memberships are.

The authoritative source for compiling this list was searching the codebase for out_of_band_membership = True (there are four such hits, corresponding to cases (1), the federation and local cases for (2), and (3)). I could add such a list to is_out_of_band_membership, or the DAG concepts doc, but it could run the risk of getting out of date.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that makes sense. Perhaps just the introductory sentence from https://matrix-org.github.io/synapse/develop/development/room-dag-concepts.html#out-of-band-membership-events

A special case of outlier events are some membership events for federated rooms that we aren't full members of.

would help illuminate is_out_of_band_membership? Not going to block this on that though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the docstring for is_out_of_band_membership in 947f651.

#
# OOB memberships are used in three (and a half) situations:
#
# (1) invite events which we have received over federation. Those
# will have a `sender` on a different server, so will be
# skipped by the "is_mine" test above anyway.
#
# (2) rejections of invites to federated rooms. These are normally
# created via federation (in which case the remote server is
# responsible for sending out the rejection). If that fails,
# we'll create a leave event locally, but that's only really
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it's this local fallback event that is considered out-of-band?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, both the federated and local cases are out-of-band.

Any thoughts on how I can reword this paragraph to make this clear? How about:

                    # (2) rejections of invites to federated rooms - either remotely
                    #     or locally generated. (Such rejections are normally
                    #     created via federation, in which case the remote server is
                    #     responsible for sending out the rejection. If that fails,
                    #     we'll create a leave event locally, but that's only really
                    #     for the benefit of the invited user - we don't have enough
                    #     information to send it out over federation).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the rework. But the underlying difficulty I'm having here is that I don't grok the meaning of out of band. It makes me think of an event that has been received by this server over some non-Matrix protocol, e.g. carrier pigeon.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is maybe not the best term (see #4405 (review), which is where we expanded the idea beyond case (1).)

It's "out of band" inasmuch as it's not sent over the regular federation/v1/send API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are only two hard problems, after all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the wording here in ca5b502.

# for the benefit of the invited user - we don't have enough
# information to send it out over federation.
#
# (2a) rescinded knocks. These are identical to rejected invites.
#
# (3) knock events which we have sent over federation. As with
# invite rejections, the remote server should send them out to
# the federation.
#
# So, in all the above cases, we want to ignore such events.
#
# OOB memberships are always(?) outliers anyway, so if we *don't*
# ignore them, we'll get an exception further down when we try to
# fetch the membership list for the room.
#
if event.internal_metadata.is_out_of_band_membership():
return

# Finally, there are some other events that we should not send out
# until someone asks for them. They are explicitly flagged as such
# with `proactively_send: False`.
if not event.internal_metadata.should_proactively_send():
return

Expand Down