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

Fix possible race condition that can lead to duplicated messages #627

Merged
merged 10 commits into from
May 17, 2024

Conversation

jespino
Copy link
Member

@jespino jespino commented Apr 26, 2024

This PR follows 2 approaches to avoid possible race conditions on the message sent to MS Teams from mattermost and receiving the "echo" too soon. 1 is, double check if the message is already associated with a mattermost message. Right before creating the new message.

The other strategy applied is to introduce a non-visible html label (an empty abbr lable, with the title generated-from-mattermost that tags the message itself as something generated from mattermost without affect the look and feel of the message in MSTeams side. Then we check if that "tag" exists in the message, and if that is the case, we discard the message as it was generated from mattermost.

@jespino jespino added the 2: Dev Review Requires review by a core committer label Apr 26, 2024
Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

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

Sweet! Can we find a way to unit test this?

Copy link
Member

@sbishel sbishel left a comment

Choose a reason for hiding this comment

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

Could you explain why the <abbr> tag is being used? Also after creation, should it be removed. Will it cause any issues if the user edits the post later?

@@ -272,6 +272,10 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs
return metrics.DiscardedReasonNotUserEvent
}

if strings.HasSuffix(msg.Text, "<abbr title=\"generated-from-mattermost\"></abbr>") {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, let me move it to a constants

@jespino
Copy link
Member Author

jespino commented Apr 29, 2024

@sbishel the reason to use <abbr> is the fact that is common enough to be accepted in MSTeams without sanitizing it, and is uncommon enough to probably not collide with anything else, but at the same time, It doesn't affect the layout because is by default inline, and also support a title attribute that is useful to pass the data that we want.

There is not problem in editions, because editions are not going to be detecting this, it only affects the creations, and for editions, normally we already have the information in the server anyway, so it is not that relevant. Also, creations is overwhelmingly bigger than editions, so this optimization is very relevant.

Also, I was thinking about the posibility of include the "remote-id" or any other "server side code", to be sure that if you have 2 mattermost instances connected to the same microsoft instance, you are replicating properly everything as expected. It is not a use case that we support, but there is not harm on adding that extra data, I guess.

@jespino
Copy link
Member Author

jespino commented Apr 29, 2024

@lieut-data The tests are already verifying that the messages generated are the right ones with the new string, and also I added a new tests for verifying that the message gets discarded after that text is found in the message.

@jespino jespino added QA/Deferred Agreement with QA that these changes will be tested post-merge and removed 2: Dev Review Requires review by a core committer labels May 16, 2024
@jespino jespino merged commit ae3640f into main May 17, 2024
9 checks passed
@jespino jespino deleted the reduce-race-condition-message-duplication branch May 17, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA/Deferred Agreement with QA that these changes will be tested post-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants