This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support MSC3814: Dehydrated Devices #15929
Support MSC3814: Dehydrated Devices #15929
Changes from 13 commits
6baeda9
ace4f49
49f892d
6c183c5
6d0ce6f
b95364e
395e039
b20c4c7
a950d5e
fe9be3c
ccd6c12
664ad97
a52a25a
0f49f81
f7e0933
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a /sync next_batch stream token or a stream id? (I'd expect stream ids to be integers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned already, I think we should not delete any delivered to-device messages for dehydrated devices.
We can do this in a later PR, but I think that this will be crucial to ease the resumption of rehydration and ensure that room keys don't get lost because a device aborted the rehydration step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can open a follow-up PR to stop deleting the delivered to-device messages and address the TODOs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The follow-up PR is here: #16010
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not align with the MSC, the MSC tells us this:
So, per MSC, we should delete the device the first time the client makes a request to this endpoint, no matter how many messages we may have.
That being said, I think that we should leave the deletion of the device up to the client, see my rationale here: matrix-org/matrix-spec-proposals#3814 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is one of the things I asked for clarification about on the MSC, see matrix-org/matrix-spec-proposals#3814 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for multiple clients to be hitting this endpoint at once? What solves that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be something like a scenario where (let's say) two devices have called
GET /dehydrated_device
and received the dehydrated device id, and are now both calling this endpoint with the id?I don't think there is anything currently preventing this - iirc from the MSC proposal, this sort of scenario was the rationale for deleting the dehydrated device the first time this endpoint is called, as that functions as "claiming" the dehydrated device so it can't be used by other clients. This behavior was contentious on the MSC though - I implemented deleting the device after all the messages are delivered, but that won't help in the case where two devices are calling this endpoint at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah probably a conversation for the MSC, but seems quite likely now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what will help, and will be necessary for perfect resumption, is that we shouldn't delete the events untill the device is fully deleted.
This way any device can attempt to rehydrated the device at any point in time, even if another one is already rehydrating it, until one device succeeds and deletes the dehydrated device.