Add forward extremities endpoint to rooms admin API

[jaywink]

Enables querying and deleting forward extremities from rooms via the admin API.

When deleting, the get_latest_event_ids_in_room cache is invalidated, so that the server does not need to be restarted.

Signed-off-by: Jason Robinson <mail@jasonrobinson.me>

[clokep]

Looks reasonable overall!

[clokep]

What sort of situations would you use this in? Is the state group useful?

[jaywink]

No, probably not particularly, except maybe for developers. I did think of returning just a count, but it felt that since the information "is there", it felt unnecessary to not return results as well.

My understanding of the needs for this API endpoint from an operational point of view is mainly to see the count, though I would maybe check with the EMS ops team first to confirm.

[clokep]

I suspect that's just internal state then and not worth returning. @erikjohnston do you see any value in returning those?

[erikjohnston]

I've sometimes used the state group as a proxy for how big the state difference was between extremities were, but it's a bad heuristic. I think a more useful one would probably be depth and received_ts, as that'll give you a better way of looking for stuck extremities

[jaywink]

Do we consider those to be useful in this admin API? I guess the options are:

1. keep as is
2. remove state_group
3. remove state_group, add depth and received_ts
4. remove everything, just return a count
5. something else? :)

I don't have enough experience on the extremities to vote really.

[erikjohnston]

I think adding depth and received_ts is probably the right thing

[clokep]

I think we usually try to not raise synapse errors from the database layer, but I'm not finding a much better solution at the moment.

[jaywink]

Yeah I felt annoyed at this too, but didn't feel there was much to do, without separating the queries into separate transactions at least. I guess one thing could be raising some non-http error and then catching that in the servlet to raise a SynapseError? I didn't do it since I saw some other places use the SynapseError in db code.

[clokep]

Is there a current issue that this is working around?

[erikjohnston]

A couple of notes:

1. This should have a big health warning on it, as if its called without thought it can lead to room takeover attacks where someone can change the servers view of the current state
2. There's a race where this won't work for busy rooms, as we read the extremities in one transaction and then write them in another (this happens if you have really busy room)

[jaywink]

1. This should have a big health warning on it, as if its called without thought it can lead to room takeover attacks where someone can change the servers view of the current state

Hmm, I'm wondering what kind of warning we can put while still enabling this kind of API call, if there is potential for users to mess up their servers?

On the other hand, this is something that EMS ops has needed to do sometimes, which is why this PR exists to make the operations easier.

Just a generic "WARNING: Please ensure you know what you're doing and have read the related issue #1760 " etc?

2. There's a race where this won't work for busy rooms, as we read the extremities in one transaction and then write them in another (this happens if you have really busy room)

For this PR I had to dig into how the db layer works to understand when commits happen and I was under the understanding (from code) that this will all happen in one transaction? We do two selects but AFAICT the commit happens only after both of them, given both statements are done inside a single runInteraction call?

Note the GET API is just separate to get some kind of generic status on the situation. Not all admins will have for example Prometheus metrics enabled. Of course ideally a GET endpoint to get "rooms with most extremities" might be more useful, but I was going to do that possibly next.

[erikjohnston]

1. This should have a big health warning on it, as if its called without thought it can lead to room takeover attacks where someone can change the servers view of the current state

Hmm, I'm wondering what kind of warning we can put while still enabling this kind of API call, if there is potential for users to mess up their servers?

On the other hand, this is something that EMS ops has needed to do sometimes, which is why this PR exists to make the operations easier.

Just a generic "WARNING: Please ensure you know what you're doing and have read the related issue #1760 " etc?

That'd probably be fine, plus saying that the API should absolutely not be run automatically.

1. There's a race where this won't work for busy rooms, as we read the extremities in one transaction and then write them in another (this happens if you have really busy room)

For this PR I had to dig into how the db layer works to understand when commits happen and I was under the understanding (from code) that this will all happen in one transaction? We do two selects but AFAICT the commit happens only after both of them, given both statements are done inside a single runInteraction call?

Note the GET API is just separate to get some kind of generic status on the situation. Not all admins will have for example Prometheus metrics enabled. Of course ideally a GET endpoint to get "rooms with most extremities" might be more useful, but I was going to do that possibly next.

We precompute the extremities and state and then call the persist events transaction, c.f. https://github.com/matrix-org/synapse/blob/develop/synapse/storage/persist_events.py#L414-L419

[clokep]

Looks like @erikjohnston left some feedback that needs to be handled already, so I'm going to mark this as request changes.

[jaywink]

Have updated 👍

[jaywink]

Not entirely sure if the test failure is related to the PR?

[clokep]

Not entirely sure if the test failure is related to the PR?

It is not, but can you merge develop into this branch to fix it?

[jaywink]

It is not, but can you merge develop into this branch to fix it?

Done.

[erikjohnston]

I think otherwise this looks OK

[clokep]

I merged develop again, which should fix the build issue.

[jaywink]

Conflict resolved and PR review addressed.

[erikjohnston]

I think this looks good now, thanks!

[jaywink]

Merging as @clokep 's "changes requested" review was about Erik's comments, which were addressed and approved. Thanks for the review!