Move RestartDelayedEventServlet to workers

[anoadragon453]

To allow delayed event timeouts to be extended even if the main process is currently restarting (for instance, during a pod resize). Addresses the /restart portion of #19141.

Note that this does not move the older, general POST /org.matrix.msc4140/delayed_events/{delayID} endpoint to workers. That endpoint had the caller specify {restart,send,cancel} as a JSON body parameter. As such, clients will need to ensure they're using /restart to get the benefit of worker request handling.

Part of MSC4140

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct (run the linters)

[anoadragon453]

This routes the endpoint to workers when running the relevant Complement tests.

[devonh]

I think the changes here are okay, but I'm mostly taking you at your word that this comment is true.

Would you be able to provide a more thorough explanation / links to the relevant code which validates this?

[anoadragon453]

Ah sorry. See the code just below where self._initialized_from_db is set. I failed to realise that someone reading this from top-to-bottom wouldn't yet know what self._initialized_from_db was.

Essentially _initialized_from_db is a deferred that completes once all pending delayed events in the DB have been scheduled. The main process will manage that. The restart handler will update the timeout in the DB, so is not dependent

It's probably simpler if await make_deferred_yieldable(self._initialized_from_db) below is only called on the main process. I've made that visual change in 6f6227c.

We're effectively letting the restart handler code on workers and _schedule_db_events on the main process run in parallel. I had a think about whether this would result in a race condition.

The restart handler calls restart_delayed_event, which updates a delayed event's timeout as long as it hasn't been marked as processed (is_processed) yet.

is_processed means "sending of this event has begun". Once a delayed event has been successfully sent, it is deleted from the delayed_events table.

I don't believe there's a point between the queries in _schedule_db_events where running UPDATE delayed_events SET send_ts = ? + delay WHERE delay_id = ? AND NOT is_processed would be detrimental.

The only thing that stands out to me is between self._store.unprocess_delayed_events and self._store.process_timeout_delayed_events, when all events marked as "processed" in the DB are set to unprocessed again (because processing may have failed partway during homeserver shutdown). But in that case, the event in question will just have its send_ts successfully extended and self._store.process_timeout_delayed_events won't send it yet, which sounds like what the client intended anyhow.

And then given that, if the restart code can't race with the initialisation code anyhow, we could just remove await make_deferred_yieldable(self._initialized_from_db)fromrestart` altogether, even when running on the main process.

cc @AndrewFerr who originally wrote this code.

[AndrewFerr]

The only thing that stands out to me is between self._store.unprocess_delayed_events and self._store.process_timeout_delayed_events, when all events marked as "processed" in the DB are set to unprocessed again (because processing may have failed partway during homeserver shutdown). But in that case, the event in question will just have its send_ts successfully extended and self._store.process_timeout_delayed_events won't send it yet, which sounds like what the client intended anyhow.

There is a slight amount of "destructiveness" here. The startup also handles sending delayed events that should have been sent while the homeserver was offline, i.e. delayed events with an effective send time in the past. Calling restart on such a delayed event during this startup would prevent it from being sent as it ought to be.

[AndrewFerr]

Here's an idea: instead of the startup calling unprocess_delayed_events before process_timeout_delayed_events (so that the latter will retry "processed" sends that failed to persist), process_timeout_delayed_events could simply have a flag for whether it should skip over "processed" events or not (which the startup would set to "not skip").

The scenario in my last post is possible only because calling unprocess_delayed_events on startup makes "past" delayed events eligible targets of restart. This issue would go away if the startup were to retry past delayed events without having to alter the "processed" state of delayed events in the DB.

[AndrewFerr]

If you want, a worker handling restart may skip calling _schedule_next_at and instead only update the DB with the new send time of the restarted delayed event. The main process will notice the new send time on its next scheduled _send_on_timeout.

Currently, if a worker schedules calls of _send_on_timeout, those calls will often end up being scheduled at the same time as the main process' calls of it, i.e. the worker & main process will often both try to send the same delayed event. Only one of them will "win" at a time, so the work of actually sending a delayed event won't be repeated between them, but there isn't any explicit load balancing done to try to spread that work evenly. So maybe it's better to keep the main process as the sole manager of scheduled sends.

[anoadragon453]

That certainly sounds easier to reason about. I walked through an example to convince myself that this would still result in events being sent on time:

• User says they want to send an event at timestamp 10.
• At timestamp 8, the user pushes the send time to 20 on a worker.
• The main process still tries to send the event at 10. It's no longer there. Now schedules a timer for 20.
• Main process sends the event at timestamp 20.

Since we're always pushing the event forwards, and calling _send_on_timeout without any events to send only results in some small overhead, I see no issue with your proposal. Updated in 66f026e.

[devonh]

Awesome, this looks a lot better now with easier to understand rationale for why we can rely on certain behaviour. :)