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

Clear pending commit state after commit validation fails #820

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

cameronvoell
Copy link
Contributor

@cameronvoell cameronvoell commented Jun 6, 2024

Fixes #805

Changes for preventing groups being stuck in a pending-commit state:

  1. when a staged commit fails validation, clear pending commit state and set intent state to failed =>
    https://github.com/xmtp/libxmtp/pull/820/files#diff-202bbf85eac4d977447dc52cf6f01c12b56e9b8e72e579844bfa5bc43862acecR233-R243
  2. do not return an error for functions passed into process_for_id if they contain db transactions you do not wish to be rolled back
    https://github.com/xmtp/libxmtp/pull/820/files#diff-202bbf85eac4d977447dc52cf6f01c12b56e9b8e72e579844bfa5bc43862acecR244-R246
  3. do not retry intents with IntentState::Error, instead return an error
    https://github.com/xmtp/libxmtp/pull/820/files#diff-202bbf85eac4d977447dc52cf6f01c12b56e9b8e72e579844bfa5bc43862acecR154-R161

@cameronvoell cameronvoell marked this pull request as ready for review June 6, 2024 17:46
@cameronvoell cameronvoell requested a review from a team as a code owner June 6, 2024 17:46
"not retrying intent ID {}. since it is in state Error",
intent.id,
);
return Err(last_err.unwrap_or(GroupError::Generic(
Copy link
Contributor

@insipx insipx Jun 6, 2024

Choose a reason for hiding this comment

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

It would be better to make errors like this into concrete enum variants like GroupError::CouldNotBeCommitted with the message as an #[error("Group Intent could not be committed")] rather than throwing everything into the Generic bucket. Should make these errors easy to match on should we need to, + can use the Retryable trait on them which gives us finer grained control over what will/will not be retried

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@insipx I agree, I added a comment on the follow up issue here #822 (comment).

Think it's better to update this error message while looking at that issue so that this category of errors are more cohesive/ better organized. Thanks for the idea though, I'll make sure this gets updated 🙏

Copy link
Contributor

@insipx insipx left a comment

Choose a reason for hiding this comment

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

looks good to me other than error nit!

@cameronvoell cameronvoell merged commit 748866b into main Jun 6, 2024
6 checks passed
@cameronvoell cameronvoell deleted the cv/metadata-update-fails-after-failed-commit branch June 6, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants