Faster joins: Avoid starting duplicate partial state syncs

[squahtx]

Currently, we will try to start a new partial state sync every time we
perform a remote join, which is undesirable if there is already one
running for a given room.

We intend to perform remote joins whenever additional local users wish
to join a partial state room, so let's ensure that we do not start more
than one concurrent partial state sync for any given room.

---

There is a race condition where the homeserver leaves a room and later
rejoins while the partial state sync from the previous membership is
still running. There is no guarantee that the previous partial state
sync will process the latest join, so we restart it if needed.

Signed-off-by: Sean Quah seanq@matrix.org

---

Best reviewed commit by commit.

[DMRobertson]

Seems sane, though I don't completely follow.

Would it be feasible (worthwhile?) to add some short trial test for this?

When we are kicked from a room while syncing partial state, the sync
can fail

because the server we're trying to sync from may now refuse /state_ids calls?

and leave us in a state where the homeserver is not joined to
the room, yet has the partial state flag set on the room.

Is the point that in this state there may still be a resync process running, because we may not yet have this this logic?

synapse/synapse/handlers/federation.py

Lines 1772 to 1792 in 54c012c

	except FederationError as e:
	# TODO: We should `record_event_failed_pull_attempt` here,
	# see https://github.com/matrix-org/synapse/issues/13700
	if attempt == len(destinations) - 1:
	# We have tried every remote server for this event. Give up.
	# TODO(faster_joins) giving up isn't the right thing to do
	# if there's a temporary network outage. retrying
	# indefinitely is also not the right thing to do if we can
	# reach all homeservers and they all claim they don't have
	# the state we want.
	# https://github.com/matrix-org/synapse/issues/13000
	logger.error(
	"Failed to get state for %s at %s from %s because %s, "
	"giving up!",
	room_id,
	event,
	destination,
	e,
	)
	raise

[squahtx]

Would it be feasible (worthwhile?) to add some short trial test for this?

I've added some tests for the deduplication and restart logic.

When we are kicked from a room while syncing partial state, the sync can fail

because the server we're trying to sync from may now refuse /state_ids calls?

Yes.

and leave us in a state where the homeserver is not joined to
the room, yet has the partial state flag set on the room.

Is the point that in this state there may still be a resync process running, because we may not yet have this this logic?

I'm not sure I follow the question. The resync process will not be running, because it would have raised an uncaught exception and terminated if every homeserver in its list failed to provide the state before a given partial state event.

[squahtx]

I've redone the PR description and code comments in an attempt to make them clearer. Let me know if they're any better or worse.

[DMRobertson]

I've redone the PR description and code comments in an attempt to make them clearer. Let me know if they're any better or worse.

Many thanks for this (and the tests)---I appreciate the explicitness!

I'm not sure I follow the question.

I'm not sure what I was getting at either.

I think I was trying to understand how we could end up with duplicate resync processes today, and wondered if this had something to do with the "total failure to handle resync" (#13000). The updated description helps a lot though, many thanks!

[squahtx]

Thanks for the review! I'm glad that it makes more sense this time around.