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.
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
Receive flow for application messages #326
Receive flow for application messages #326
Changes from 11 commits
b091059
82d1eb7
d88e3f4
9205886
b02f5a8
97db939
aa623dd
8654670
be31694
563c6c9
17f5fcb
8829ac8
3d84f9c
eaf466a
b288c47
dd77029
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.
The thing that gets messy here is error handling. There are some errors we want to just log and move on from (garbage message, conflicting epochs/race conditions).
But there are some errors that we need to retry. For example, any sort of IOError saving to the DB or getting a connection from the pool. If Client A processes a commit successfully and Client B fails on the same commit, their group state will fall out of sync and Client B will be unable to read future messages.
One way to solve this would be to have some sort of
IsRetryable
method available onGroupError
s. Retryable errors would abort theprocess_messages
flow entirely, since we don't want to apply any future commits or increment thelast_message_timestamp
until we've fully processed the preceding message. The caller could then retry later. We'd just have to be sure that retryable errors can eventually either succeed or turn into non-retryable errors (maybe by keeping track of the number of attempts). Otherwise it could be a never-ending retry loop.Open to other solutions.
It also might be valid to completely ignore this edge case and just have really good mechanisms for re-adding yourself to the group once the client gets into this bad state, assuming the bad state is reliably detectable.
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 is a great point - I added this to the list of things to follow up on above
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 have a similar issue to figure out for the message publishing flow as well.
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 OpenMLS manual states:
To guarantee the best possible Forward Secrecy, the key material used to encrypt messages is immediately discarded after encryption. This means that the message author cannot decrypt application messages. If access to the messages content is required after creating the message, a copy of the plaintext message should be kept by the application.
So I think we'll need to do a few things in the commit flow:
Saving these for later PR's.
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 plaintext and the message hash are already saved in the intent to tee this flow up
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.
For application messages, we can just use the plaintext saved in the intent.
For commits, we just need to
merge_pending_commit
. Technically, there is some ambiguity around which pending commit is actually being merged since the function doesn't take any arguments. We might want some more metadata on the intent to ensure the pending commit in the mls store matches the intent. In practice it should be fine since we can only have one pending commit at a time.As long as we properly clear out failed intents it'll always be committing the right one, but it does feel a bit brittle to not know exactly which staged commit you are merging.
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.
@neekolas it might make sense to save the plaintext during intent publishing directly onto the messages table, with a 'status' field like 'uncommitted', that allows us to support optimistic UI on send if apps want it
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.
Agreed. We are going to need optimistic sending. Especially if we don't intend to fully sync after every message publish, which I think makes the most sense.