Add new columns tracking when we partial-joined

[DMRobertson]

First half of #13891. A contribution to #12993.

This information will let us work out which servers to send device list updates to once we have fully joined the room. Doing so will ensure that we correctly fulfil our obligation to notify other HSes of our users' device list changes.

[DMRobertson]

I haven't run this against Sean's complement tests branch, but given that this doesn't change any user-visible behaviour I don't think we particularly need to?

[DMRobertson]

sqlite3.IntegrityError: FOREIGN KEY constraint failed

Oh dear. Have we not yet persisted send join event?

[squahtx]

sqlite3.IntegrityError: FOREIGN KEY constraint failed

Oh dear. Have we not yet persisted send join event?

Not at the time we create the partial_state_rooms entry.

[DMRobertson]

Not at the time we create the partial_state_rooms entry.

Hmm. I could drop the FOREIGN KEY constraint, but I'd be sad to do so. If we want to obey the constraint, it looks like I'd need to call store_partial_state_room after process_remote_join---maybe even at the top of the finally block? (I'm not 100% clear under which circumstances we hit PartialStateConflictError.)

It doesn't look like process_remote_join needs the DB row that store_partial_state_room will write... but I'm not 100% sure.

synapse/synapse/handlers/federation.py

Lines 582 to 630 in de0002b

	if ret.partial_state:
	# Mark the room as having partial state.
	# The background process is responsible for unmarking this flag,
	# even if the join fails.
	await self.store.store_partial_state_room(
	room_id,
	ret.servers_in_room,
	ret.event.event_id,
	self.store.get_device_stream_token(),
	)
	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)
	finally:
	# Always kick off the background process that asynchronously fetches
	# state for the room.
	# If the join failed, the background process is responsible for
	# cleaning up — including unmarking the room as a partial state room.
	if ret.partial_state:
	# Kick off the process of asynchronously fetching the state for this
	# room.
	run_as_background_process(
	desc="sync_partial_state_room",
	func=self._sync_partial_state_room,
	initial_destination=origin,
	other_destinations=ret.servers_in_room,
	room_id=room_id,
	)

[squahtx]

I believe we have to write to partial_state_rooms before, or in the same transaction as persisting the join event. Otherwise if we were to crash after persisting the join event, we would think we had a full state room and never try to sync the full state.

I'm also not clear on whether it's possible to hit a PartialStateConflictError here. If we do, I think something's gone very wrong.

[DMRobertson]

I believe we have to write to partial_state_rooms before, or in the same transaction as persisting the join event.

Agreed on this.

I don't fully grok Synapse's database machinery, but AFAIK persisting an event might happen on a different worker. So I don't think it'll be easy to make write partial_state_rooms take place in the same transaction as the event persistence. Given that, it's probably easiest to drop the foreign key constraint. WDYT?

[squahtx]

I don't fully grok Synapse's database machinery, but AFAIK persisting an event might happen on a different worker. So I don't think it'll be easy to make write partial_state_rooms take place in the same transaction as the event persistence. Given that, it's probably easiest to drop the foreign key constraint. WDYT?

I think you're right about it happening on a different worker. It does sound easiest to drop the constraint, if a little sad.

[erikjohnston]

Suggested change

	ALTER TABLE partial_state_rooms ADD COLUMN device_lists_stream_id BIGINT;
	ALTER TABLE partial_state_rooms ADD COLUMN device_lists_stream_id BIGINT NOT NULL;

[DMRobertson]

What's the appropriate value to use for a default?

[erikjohnston]

OH, because there might be existing entries. Hmm. I think 0 should be fine. It means we'll resend out all device list updates, but nothing should actually be using partial joins yet

[erikjohnston]

Can we insert null here, and then update it with the join event ID in the persist event DB transaction? That way we might be able to keep the foreign key constraint?

[DMRobertson]

It'd mean that we have to tell the persist event logic "you're persisting a partial join response" somehow. I can look at doing this but I'm not sure if it's worth the extra complexity in the persistence path. WDYT?

[DMRobertson]

I could use a hand writing a test which checks that we correctly write the join_event_id.

[erikjohnston]

Why do we do this in an else block rather than in the try or afterwards?

[DMRobertson]

My thinking was that

• If process_remote_join fails then the event may not be persisted
• keeping the try block small helps clarify what the PartialStateConflictError is catching

Happy to just bung it at the bottom of the try though if you prefer.

[DMRobertson]

Looks like I've broken something, judging by complement

[erikjohnston]

This shouldn't be an async def

[erikjohnston]

Also wrong function

[DMRobertson]

oh bugger. Why didn't mypy spot that?

[DMRobertson]

Because we don't use ParamSpec in synapse.storage.database.py. Will refrain for now but I've found my next brain cleaner.

[erikjohnston]

gogogogoggo