Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

createRoom should be atomic #1951

Open
nico-famedly opened this issue Sep 17, 2024 · 4 comments
Open

createRoom should be atomic #1951

nico-famedly opened this issue Sep 17, 2024 · 4 comments
Labels
improvement An idea/future MSC for the spec

Comments

@nico-famedly
Copy link
Contributor

nico-famedly commented Sep 17, 2024

Currently createRoom can fail for various reasons, which will cause the homeserver to create a partial room. This confuses clients, since they will receive an error and no room id or they receive no error and a room id. While some of that is because of how servers implement it, there is at least one case which is unavoidable:

If you include multiple invites during room creation, some of those might fail because of the remote server rejecting the invite or simply being unavailable. This is not something that can be validated before creating the room, since the room id needs to be included in the invite. Additionally for multiple invites, the behaviour is not clear when the first invite succeeds and later invites fail.

There are a few ways this can be solved:

  • Disallow invites completely on createRoom. This is probably not ideal for DMs.
  • Disallow multiple invites/only allow a single invite. In that case the homeserver can verify that the invite succeeds and only then respond to createRoom.
  • Have invites during createRoom require a multistep process, where the invite is announced to the remote homeserver first and only committed later.
  • Allow returning success and a list of failed invites to the clients, which would not make the API atomic, but allow clients to know about the error.

Additionally it would be great if the spec was clear on if a room is created, when an error is returned or not.

@nico-famedly nico-famedly added the improvement An idea/future MSC for the spec label Sep 17, 2024
@clokep
Copy link
Member

clokep commented Sep 17, 2024

Not directly related, but I wonder if invites in general should be able to fail? Should member events transition off of the invite state if the remote server can't be reached?

@nico-famedly
Copy link
Contributor Author

Since invites don't have any retry mechanism, I think it is good to have some feedback, that the invite wasn't received. This probably depends on how the invite is sent: Should invites written directly to the room state behave differently that invites sent via the api, which may only create the invite after the remote returned a positive response code?

That's probably quite the rabbit hole to investigate, but I think in general you should also be able to tell a remote server, that an invite was revoked, when you delete a room and similar, so that you don't get stuck with the current invites that can't be joined.

@richvdh
Copy link
Member

richvdh commented Sep 17, 2024

I think this is more an implementation bug than a spec bug, though I take the point that changes to the spec could make it easier to implement reliably.

Cf element-hq/synapse#8895, for example

@nico-famedly
Copy link
Contributor Author

Some of it is an implementation bug, but with the current spec there is no way to return an error, when an invite fails, and still return a roomid. And you can't revoke an invite, if the first one succeeds and the second one fails, so you either delete the room after the second invite fails and have a dangling first invite on the remote, or you create a room with some successful invites and the client needs to check if any of them failed by manually verifying the room state, even though create room returned success. Or you return an error and the client won't know what room it just created.

The current spec does neither specify what should happen in that case nor is either of the behaviours, while possibly compatible with the spec, a good UX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement An idea/future MSC for the spec
Projects
None yet
Development

No branches or pull requests

3 participants