Consider supporting all endpoints for delayed events (MSC4140) across workers

[Half-Shot]

Description:

Currently delayed events can be created on any worker but only changed on the master process. This is unfortunate as the bulk of traffic for delayed events (at least as far as Matrix RTC is concerned) involves pushing back a delayed event until a client has stopped using the application.

This has the unfortunate side effect that Element Call users for instance will send N delayed event requests per participant every few seconds, which can cause problems when the master process is otherwise busy. The typical action of a delayed delayed event request is to quickly try to schedule more to prevent an event from sending, as it's a time sensitive operation.

Instead, it would be good if both the updating of delayed events and the actual execution of sending them could be spread across workers to reduce the load on the master process and to allow better recovery and scaling of RTC-based services.

[anoadragon453]

The Element Call team have asked Backend to support solely the POST /_matrix/client/v1/delayed_events/{delay_id}/restart endpoint on workers, as this is what is used for EC's "heartbeat" functionality.

The reason this was initially only available on the main process is the following functionality of MSC4140:

Cancelling delayed state events by a more recent state event

If a new state event is sent to the same room at the same entry (event_type, state_key pair) as a delayed event by a
different matrix user, any delayed event for this entry (event_type, state_key pair) is cancelled.

The idea was that the stream of delayed events would need to be linearised at the point of receiving any request meant to manage it, such that the handling worker would know whether to cancel a prior delayed event (eliminating the race condition of two conflicting events being scheduled simultaneously).

Recently, this requirement has been dropped (moved to the Alternatives section of the MSC) in favour of using MSC4354: Sticky Events instead. So it's no longer a concern.

In addition, I've since realised that this wasn't a blocker to moving /restart off the main process anyways, as all it's doing is bumping a timeout in the DB. The point where you'd worry about cancelling conflicting prior delayed events is when you're sending the delayed event. The /restart endpoint handler isn't doing that.

(Note that the restart/cancel/send endpoints were all one endpoint before #19152, which probably made moving just the restart functionality off the main process much more difficult to reason about.)

---

The other gotcha is that each of these handlers is also doing something else. Looking at the restart handler:

synapse/synapse/handlers/delayed_events.py

Lines 419 to 437 in 7a96603

	async def restart(self, request: SynapseRequest, delay_id: str) -> None:
	"""
	Restarts the scheduled delivery of the matching delayed event.
	Raises:
	NotFoundError: if no matching delayed event could be found.
	"""
	assert self._is_master
	await self._delayed_event_mgmt_ratelimiter.ratelimit(
	None, request.getClientAddress().host
	)
	await make_deferred_yieldable(self._initialized_from_db)
	next_send_ts = await self._store.restart_delayed_event(
	delay_id, self._get_current_ts()
	)
	if self._next_send_ts_changed(next_send_ts):
	self._schedule_next_at(next_send_ts)

The

        if self._next_send_ts_changed(next_send_ts):
            self._schedule_next_at(next_send_ts)

portion is calculating when to send the next scheduled delayed event, and scheduling an in-memory timer to do so. Surely copying this across multiple workers would then be a disaster and we need to redesign everything so workers can communicate?!

Well, not really. _schedule_next_at schedules the _send_on_timeout method to run after a timeout, and that method calls process_timeout_delayed_events, which - at the point of running - checks the DB to see which delayed events have crossed their timeout and returns those that have for sending.

If /send is called on worker A, and /restart is called on worker B, then worker B will call _send_on_timeout at the correct time. but worker A will just... not send any events. No duplicate events, no problem.

---

Thus I think we can definitely move /restart to workers with little issue, which should unblock the VoIP team. From there, I'll see whether the other endpoints can be moved as well (though this is lower priority).

cc @fkwp @AndrewFerr let me know if you disagree with any of the above.

[AndrewFerr]

That sounds like a correct analysis of the situation.

Also worth considering is whether all workers need to schedule _send_on_timeout, or if it's sufficient for just the main worker to schedule it (as is the case currently).

All that's crucial is to never miss calling _send_on_timeout on time. There is no risk of this as long as delayed events can only ever be modified to time out later, since there is no harm in calling _send_on_timeout too early / too often (for the reasons you mentioned re: the DB checks).

However, adding a new delayed event to be sent before all others needs at least one worker to schedule a _send_on_timeout to handle it. Since delayed events can be added by workers (which must be supported, at least because the endpoint is shared with the worker-compatible /send & /state endpoints), the case of a worker adding one is currently achieved by the replication API notifying the main worker of the send time of the added delayed event, for the main worker to schedule _send_on_timeout appropriately.

Maybe a better approach is to let any worker that adds a delayed event to have its own scheduling of _send_on_timeout, which would eliminate the need to funnel information to the main worker. AIUI the worst that could happen with this approach is that it would occasionally cause workers to call _send_on_timeout (and thus hit the DB) more often than necessary (i.e. a single timeout may get redundantly handled by multiple workers), but that would never cause a scheduled send to be missed, or get double-sent (thanks to the DB checks). And at that point, a worker-compatible /restart (and even /cancel and /send) may as well tap into scheduling _send_on_timeout too, as whatever worker it runs on may already be scheduling it.

[anoadragon453]

#19207 fixes the /restart case.