Add a Synapse Module for configuring presence update routing

[anoadragon453]

Based on #9650

At the moment, if you'd like to share presence between local or remote users, those users must be sharing a room together. This isn't always the most convenient or useful situation though.

This PR adds a module to Synapse that will allow deployments to set up extra logic on where presence updates should be routed. The module must implement two methods, get_users_for_states and get_interested_users. These methods are given presence updates or user IDs and must return information that Synapse will use to grant passing presence updates around.

A method is additionally added to ModuleApi which allows triggering a set of users to receive the current, online presence information for all users they are considered interested in. This is the equivalent of that user receiving presence information during an initial sync.

The goal of this module is to be fairly generic and useful for a variety of applications, with hard requirements being:

• Sending state for a specific set or all known users to a defined set of local and remote users.
• The ability to trigger an initial sync for specific users, so they receive all current state.

This is quite a chunky PR, though hopefully the commits should have enough explanation in each of them to get you by.

[erikjohnston]

• The ability to trigger an initial sync for specific users, so they receive all current state. (TODO)

What do you mean by this?

Something to consider: we may want to do some validation on Synapse's side to ensure modules aren't generating their own state? Or is that a feature, rather than a bug.

I don't think its worth trying to enforce TBH,

[erikjohnston]

I've also realised that that there is another place where we have to hook in, which is where /sync tries to get all other users' presence the syncing user is interested in:

synapse/synapse/handlers/presence.py

Lines 1127 to 1147 in 8bcfc2e

	@cached(num_args=2, cache_context=True)
	async def _get_interested_in(self, user, explicit_room_id, cache_context):
	"""Returns the set of users that the given user should see presence
	updates for
	"""
	user_id = user.to_string()
	users_interested_in = set()
	users_interested_in.add(user_id) # So that we receive our own presence
	users_who_share_room = await self.store.get_users_who_share_room_with_user(
	user_id, on_invalidate=cache_context.invalidate
	)
	users_interested_in.update(users_who_share_room)
	if explicit_room_id:
	user_ids = await self.store.get_users_in_room(
	explicit_room_id, on_invalidate=cache_context.invalidate
	)
	users_interested_in.update(user_ids)
	return users_interested_in

[anoadragon453]

• The ability to trigger an initial sync for specific users, so they receive all current state. (TODO)

What do you mean by this?

When another user is determined to receive presence for all users, it's crucial to be able to send them all existing presence of all local users - not just the latest updates as the proposal currently provides. Thus, either:

1. Synapse, upon detecting a change from what the module returned last, could send all current user presence to the new user. This would ensure they are always kept up to date. However, would be wildly inefficient if the module were changing the list of returned users constantly, plus I think it reduces the flexibility of the module.
2. get_rooms_and_users_for_states could return another list containing users that should receive all local user presence. That still sounds a bit finicky though, especially if we're updating the list of users to send presence to asynchronously.
3. We could add a new method to ModuleApi that allows modules to, at any point, send all local user presence to a given set of users. This would allow the module's logic to determine when this should happen, separately from when get_rooms_and_users_for_states is called.

I'm leaning towards 3. My only reservation is whether this would be useful in the generic sense but... probably?

Something to consider: we may want to do some validation on Synapse's side to ensure modules aren't generating their own state? Or is that a feature, rather than a bug.

I don't think its worth trying to enforce TBH,

Yeah, probably not. The sysadmin did decide to install that module on their own 🙂

[anoadragon453]

@erikjohnston I've gone with 3. for now and defined ModuleApi.send_local_online_presence_to(users: List[str]) -> None, which can be used to update a user on the current state of local user presence, which is useful if they've been offline for a while.

[anoadragon453]

For the implementation of ModuleApi.send_local_online_presence_to(users: List[str]) -> None, I'm envisioning an implementation along the lines of the following:

self._presence_stream = self.hs.get_event_sources().sources["presence"]
#...
def ModuleApi.send_local_online_presence_to(users: List[str]) -> None:
    for user in users:
        if self.hs.is_mine(user):
            # Modify SyncHandler._generate_sync_entry_for_presence to call
            # presence_source.get_new_events with an empty `from_key` if
            # that user's ID were in a list modified by ModuleApi somewhere.
            # That user would then get all presence state on next incremental sync.
        else:
            # Retrieve presence state for all currently online users
            presence_events = await self._presence_stream.get_new_events(
                 user, from_key=None, include_offline=False
            )
            FederationSender.send_presence([ev.state for ev in presence_events])

Technically this method is only needed for remote users (local users could just initial sync), but it's much more convenient if the module can specify both.

[erikjohnston]

Basically yeah I think that works. Instead of broadcasting all local presence states it should only send states that the given user is interested in though.

[anoadragon453]

Basically yeah I think that works. Instead of broadcasting all local presence states it should only send states that the given user is interested in though.

Oh true. And that could be every local user if used in conjunction with get_rooms_and_users_for_states.

[anoadragon453]

So this method works great for when you have a set of state updates you want to send out. But I'm increasingly realising it works terribly for when you have a user that you want to query the current state of other users for.

Where we were looking to hook in, we'd need to somehow return user IDs of users on the server that we should query presence for. get_users_for_states expects a set of UserPresenceState which at this point we don't have yet. We actually purposefully don't have that yet. At the linked call, we try to just figure out the IDs of users we want to pull presence of, instead of pulling all the presence right away. If we do that (so that we can pass UserPresenceState instances to get_users_for_states), then we lose all of the optimisation this function is aiming for (only pulling state at the very end after we've filtered out users).

We also can't just add another method that says "for a given user syncing, which users should we try to get updates for?" as the module won't know the user IDs of anyone else on the homeserver except those it's hoping to route presence to.

So I'm a bit stuck on how to support the collection of presence through /sync while honoring what get_users_for_states returns at the moment.

[anoadragon453]

Oops, I meant to link the following for where we planned to hook into the /sync code:

synapse/synapse/handlers/presence.py

Line 1089 in e55bd0e

users_interested_in = await self._get_interested_in(user, explicit_room_id)

[erikjohnston]

Hmm. Looking at the sync code I wonder if we can make _get_interested_in return a "all" value, which we can then filter after pulling out the states that have changed:

synapse/synapse/handlers/presence.py

Lines 1089 to 1117 in 0279e0e

	users_interested_in = await self._get_interested_in(user, explicit_room_id)
	user_ids_changed = set() # type: Collection[str]
	changed = None
	if from_key:
	changed = stream_change_cache.get_all_entities_changed(from_key)
	if changed is not None and len(changed) < 500:
	assert isinstance(user_ids_changed, set)
	# For small deltas, its quicker to get all changes and then
	# work out if we share a room or they're in our presence list
	get_updates_counter.labels("stream").inc()
	for other_user_id in changed:
	if other_user_id in users_interested_in:
	user_ids_changed.add(other_user_id)
	else:
	# Too many possible updates. Find all users we can see and check
	# if any of them have changed.
	get_updates_counter.labels("full").inc()
	if from_key:
	user_ids_changed = stream_change_cache.get_entities_changed(
	users_interested_in, from_key
	)
	else:
	user_ids_changed = users_interested_in
	updates = await presence.current_state_for_users(user_ids_changed)

I think?

[anoadragon453]

Possibly, however get_users_for_states currently supports routing a subset of incoming presence to some users, rather than all of the presence.

So while that would support the "all" usecase, it wouldn't work if the module wanted to only route some presence to a user.

We could just throw our hands up and make get_users_for_states only support routing all local user presence, but the generality is further diminished.

[erikjohnston]

I was thinking that we could have a function get_interested_in(user) -> Union[Set[str], All], to figure out which presence updates to fetch, and then pass those to another function to filter them down correctly (or reuse get_users_for_states).

[anoadragon453]

Right, so that proposed function would be in the module, correct? That sounds doable.

My only concern is that if get_interested_in is not implemented by the module, then get_users_for_states may not work as expected for local users. When local presence updates come in, get_users_for_states is simply used to figure out which user /sync streams to wake up. We would then query get_interested_in to determine which state events to pull.

So we'd need to trust the module developer to return similar things for both functions. We is fine and we could document that, though ideally we'd just have a single function that could be called for both cases.

[erikjohnston]

True. Though we can add strong "here be dragons" warnings.

I wonder if we can write default implementations, get_interested_in always returning all and then use get_users_for_states as the way of filtering?

[anoadragon453]

That would work... and then it would just be a performance hit if the module didn't implement get_interested_in. And worst case is the desyncronisation of results, but hopefully the documentation would do enough here to make not doing that clear.

I'll move forward with that for now, thanks!

[anoadragon453]

I was asked today about how one would implement this module for an application service, instead of a user calling /sync, and had a bit of a look through the code. I noticed a couple of things:

1. There's some code that determines whether an application service is interested in a user or a room. This is used to check whether we need to consider sending an event linked to a room/user to the application service. We don't hook into this yet. I suppose we could add a call to get_interested_users here and call it with the user_id argument set to the Application Service's sender_localpart user? get_users_for_states would still be called to filter presence updates once an application service had been declared interested.

   This could also be worked around by having the AS expand their namespace regexes, thus making them interested in a wide range of users (potentially all), but I think that's a bit hacky.

2. The ModuleApi.send_local_online_presence_to method is currently only set up to modify /sync responses. I know we're trying to discourage AS users from syncing. So we probably need to modify the method to check if the user is a known application service sender_localpart, and if so send a burst of all current user presence states in a transaction.

I'll also add some tests for application services to verify what does and doesn't already work.

[anoadragon453]

After testing locally appservices do not currently receive presence updates.

However in the interest of not complicating this PR further, I'm going to leave a note about this and aim to fix it in a future PR.

[anoadragon453]

So tests broke briefly because we were attempting to load a FederationSender in ModuleApi's __init__, which some tests call without having set send_federation: True in their config (as expected).

So I now only load it in send_local_online_presence_to and added notes everywhere cautioning that the method must only be called on workers that support federation sending.

That lead me to another thought though, in that if you configure a PresenceRouter per worker, and your module is doing things like running ModuleApi.send_local_online_presence_to when something changes externally, that's going to happen multiple times over. So modules probably need an option to prevent more than one instance trying to broadcast presence information.

Any idea where I could add a note for that, or whether there's any precedent for configuring modules per-worker thus far? (I imagine you'd just stick a presence.presence_router block in each worker config...)

[erikjohnston]

That lead me to another thought though, in that if you configure a PresenceRouter per worker, and your module is doing things like running ModuleApi.send_local_online_presence_to when something changes externally, that's going to happen multiple times over. So modules probably need an option to prevent more than one instance trying to broadcast presence information.

Hmm, I think its fine so long as send_local_online_presence_to is only called in response to the presence handler poking it, as that will then only get run on one worker? This hasn't been a problem for any modules before as they tend to only do stuff when poked by Synapse

[erikjohnston]

Did we not decide that we wanted to filter users through get_users_for_states if we returned ALL?

[anoadragon453]

We did, and this is indeed a path that does not do so! I've made this path use _filter_all_presence_updates_for_user in 2f16ed0, as well as made that function a bit cleaner.

[anoadragon453]

Hmm, I think its fine so long as send_local_online_presence_to is only called in response to the presence handler poking it, as that will then only get run on one worker?

Right, however send_local_online_presence_to was specifically designed to be called when the list of presence-receiving users was appended to. That can happen in response to something changing externally (say if an external, block-like data structure updated to include more users 😉).

I think this is fine, and the module implementer would just need to be aware and only call the module on the right workers.

[erikjohnston]

@anoadragon453 the isort is failing :)

[anoadragon453]

🤦 fixed.