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.

Sign up for GitHub

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

Jump to bottom

Fix bug: clients without prekeys are not deleted competely #2758

Merged
merged 6 commits into from
Oct 14, 2022
Merged

Fix bug: clients without prekeys are not deleted competely #2758

merged 6 commits into from
Oct 14, 2022

Conversation

stefanwire
Copy link
Contributor

@stefanwire stefanwire commented Oct 7, 2022 •
edited by smatting
Loading

https://wearezeta.atlassian.net/browse/FS-966

This PR changes the behaviour when clients without prekeys are detected during prekey claiming.

Without this PR: a client without any prekeys is deleted from other services synchronously, but not from brig.
With this PR: a client without any prekeys is deleted

  1. synchronously from brig: This leads to the client not be listed anymore. Without this change the client's prekeys are attempted to be fetched over and over again also triggering the deletion attempts over and over again.
  2. asynchronously from all other services: This should improve response times when claming prekeys. The removal of clients from the galley service can be particularly slow for MLS clients. with this change it won't block prekey claiming

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@stefanwire stefanwire temporarily deployed to cachix October 7, 2022 14:37 Inactive
@stefanwire stefanwire temporarily deployed to cachix October 7, 2022 14:37 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 7, 2022
@smatting smatting temporarily deployed to cachix October 13, 2022 09:44 Inactive
@smatting smatting temporarily deployed to cachix October 13, 2022 09:44 Inactive
@smatting smatting changed the title replace Intra.rmClient by execDelete Fix bug: clients without prekeys are not deleted competely Oct 13, 2022
@smatting smatting temporarily deployed to cachix October 13, 2022 12:27 Inactive
@smatting smatting temporarily deployed to cachix October 13, 2022 12:27 Inactive
@smatting smatting temporarily deployed to cachix October 13, 2022 12:46 Inactive
@smatting smatting temporarily deployed to cachix October 13, 2022 12:46 Inactive
@smatting smatting marked this pull request as ready for review October 13, 2022 13:03
pcapriotti
pcapriotti approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@pcapriotti pcapriotti 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, but unfortunately a lot of polysemy-related changes are mixed in, making it difficult to see what exactly has changed.

@smatting smatting temporarily deployed to cachix October 14, 2022 16:24 Inactive
@smatting smatting temporarily deployed to cachix October 14, 2022 16:24 Inactive
@smatting smatting merged commit 6d97233 into develop Oct 14, 2022
@smatting smatting deleted the FS-966/rmclient branch October 14, 2022 16:24
This was referenced Nov 2, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcapriotti pcapriotti pcapriotti approved these changes

Assignees
No one assigned
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stefanwire @pcapriotti @smatting @zebot @mdimjasevic