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

Do not allow overriding an intent that is already published #1416

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

cameronvoell
Copy link
Contributor

Before this change parallel executing threads could calculate two different payload_hash for the same intent id and the second executing thread could override the first's intent in the DB even though it was already published to the network.

for intent in intents {
let result = retry_async!(
Retry::default(),
(async {
self.get_publish_intent_data(provider, &mut openmls_group, &intent)
.await
})
);
match result {
Err(err) => {
tracing::error!(error = %err, "error getting publish intent data {:?}", err);
if (intent.publish_attempts + 1) as usize >= MAX_INTENT_PUBLISH_ATTEMPTS {
tracing::error!(
intent.id,
intent.kind = %intent.kind,
inbox_id = self.client.inbox_id(),
installation_id = %self.client.installation_id(),
group_id = hex::encode(&self.group_id),
"intent {} has reached max publish attempts", intent.id);
// TODO: Eventually clean up errored attempts
provider
.conn_ref()
.set_group_intent_error_and_fail_msg(&intent)?;
} else {
provider
.conn_ref()
.increment_intent_publish_attempt_count(intent.id)?;
}
return Err(err);
}
Ok(Some(PublishIntentData {
payload_to_publish,
post_commit_action,
staged_commit,
})) => {
let payload_slice = payload_to_publish.as_slice();
let has_staged_commit = staged_commit.is_some();
provider.conn_ref().set_group_intent_published(
intent.id,
sha256(payload_slice),
post_commit_action,
staged_commit,
openmls_group.epoch().as_u64() as i64,
)?;

This was causing an intent that was already on the network to be overwritten locally, causing the incoming message to be interpreted as an external message, which would cause the sender of the intent to be forked from the rest of the group, this was shows in tests on the following React Native PRs:

After this change the parallel executing commit messages in React Native do not cause the forked state.

@cameronvoell cameronvoell requested a review from a team as a code owner December 14, 2024 01:00
@insipx
Copy link
Contributor

insipx commented Dec 14, 2024

iirc, the change from just ToPublish to ToPublish and Published was because of a different race condition, but I don't remember exactly if it was a real issue, or something just transient that did not effect the user (PR). It seems like maybe a non-issue now

Copy link
Contributor

@codabrink codabrink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants