Faster joins: don't stall when a user joins during a fast join

[MatMaul]

Fixes #12801.
Complement tests are at
matrix-org/complement#567.

Avoid blocking on full state when handling a subsequent join into a
partial state room.

Also always perform a remote join into partial state rooms, since we do
not know whether the joining user has been banned and want to avoid
leaking history to banned users.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[MatMaul]

We had a discussion about do_invite_join here.
TLDR:

1. this method currently removes current room extremities as a first step. If the second send_join is not accepted by the resident server, we don't get any new extremity and we can't send messages anymore.
2. un-partial-stating during this is not handled and should fail

For 1 we could store the membership event received from send_join (if succeeded) in the same transaction as deleting the old extremities.

[squahtx]

One of the ways an un-partial-stating during join could manifest is in a PartialStateConflictError (note the todo).

synapse/synapse/handlers/federation.py

Lines 643 to 665 in 54c012c

	try:
	max_stream_id = (
	await self._federation_event_handler.process_remote_join(
	origin,
	room_id,
	auth_chain,
	state,
	event,
	room_version_obj,
	partial_state=ret.partial_state,
	)
	)
	except PartialStateConflictError as e:
	# The homeserver was already in the room and it is no longer partial
	# stated. We ought to be doing a local join instead. Turn the error into
	# a 429, as a hint to the client to try again.
	# TODO(faster_joins): `_should_perform_remote_join` suggests that we may
	# do a remote join for restricted rooms even if we have full state.
	logger.error(
	"Room %s was un-partial stated while processing remote join.",
	room_id,
	)
	raise LimitExceededError(msg=e.msg, errcode=e.errcode, retry_after_ms=0)

We also need to be careful that we don't return a room to partial state, because that breaks a lot of assumptions.

[MatMaul]

1. this method currently removes current room extremities as a first step. If the second send_join is not accepted by the resident server, we don't get any new extremity and we can't send messages anymore.

For 1 we could store the membership event received from send_join (if succeeded) in the same transaction as deleting the old extremities.

It's not possible since persisting events can potentially be done in another worker.

What I've done instead to mitigate the issue:

• do not remove forward extremities if the room is partially joined (in SQL to avoid races)
• move _clean_room_for_join call after send_join, so we avoid cleaning up if the join is failing anyway

[MatMaul]

I've added a way to fallback to local join if a room is unpartial stated during remote join.

However I am not sure this is really useful: retrying the remote join is a lot simpler, and I think that should be enough ?

Actually it's possible we will never hit the case: the event context is calculated by another resident server in a make_join remote call. It can contain local state (if a message was send to the resident server from the joining-already-joined server in the meantime), but unsure if/how this plays out with the local unpartial state, since it's coming from the remote resident server.

I think we should just retry the whole make/send_join in case of and that should be enough, in the same principle as 54c012c.

[squahtx]

I tried to address the issues with restricted join in #14882, but it seems to have ended up second join-proofing do_invite_join and replacing that part of this PR. Please ignore all my comments about that method if #14882 lands.

[squahtx]

Now that #14882 has landed, I've gone and resolved the merge conflicts.

All the PR comments plus changes for do_invite_join have gone into #14882, so that method is now compatible with second joins.
Also do_invite_join will no longer raise a PartialStateConflictError because there is locking now.

[squahtx]

I think knocking (line +1052) is going to be broken in that we can end up sending a knock event from a user with a ban we don't know about.
We can't exactly await full state, since we're holding the room membership lock. Any thoughts on what we should do in this case?

[DMRobertson]

we can end up sending a knock event from a user with a ban we don't know about.

Isn't it possible for us to be in this situation already, if we try to knock on a room that this HS isn't joined to?

I'm tempted to ignore this case and let fully-joined homeservers in the room reject the event. (Partially-joined HSes may accept the knock if they too have not seen the ban, but the resync process should clear that up?)

[squahtx]

It's a bit different. Previously, we would do a remote /make_knock+/send_knock and the remote homeserver would tell us to go away if we were banned.
Now, since we are in the room but do not have full state, we can add a local knock into the DAG that would otherwise be rejected and cite it as a prev/auth event.

[DMRobertson]

Hmm. Would it be worth requiring knocks to go over federation during the resync? (Not necessarily in this PR)

[squahtx]

I'm not wholly convinced the remote knock code is suitable. It persists the knock event as an outlier. I'm not sure what happens if we then receive the knock event over federation.

[DMRobertson]

I'm inclined to land this as-is and kick any knocking cans down the road.

[DMRobertson]

I think this LGTM and we should land it, provided that we have reasonable complement coverage to land with it.

[DMRobertson]

we can end up sending a knock event from a user with a ban we don't know about.

Isn't it possible for us to be in this situation already, if we try to knock on a room that this HS isn't joined to?

I'm tempted to ignore this case and let fully-joined homeservers in the room reject the event. (Partially-joined HSes may accept the knock if they too have not seen the ban, but the resync process should clear that up?)

[DMRobertson]

Ahh, checking the previous membership ensures that profile changes don't trigger another remote join. (Maybe worth writing a complement test that you can change your profile during the resync? Don't think that should block this though.)

[squahtx]

Added a test as part of matrix-org/complement#567. Synapse develop currently blocks and fails the test.

[DMRobertson]

Also: is the description of this PR still accurate to what it does, or should we update it?

[squahtx]

Also: is the description of this PR still accurate to what it does, or should we update it?

Er, let's update it.