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

ElementR| Retry query backup until it works during migration to avoid spurious correption error popup #4113

Merged

Conversation

BillCarsonFr
Copy link
Member

Fixes element-hq/element-web#27196

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@BillCarsonFr BillCarsonFr requested a review from a team as a code owner March 15, 2024 13:44
backupCallDone = true;
} catch (e) {
// Retry until successful, use simple constant delay
await sleep(2000);
Copy link
Member Author

Choose a reason for hiding this comment

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

Figured out it was better to wait a bit, not sure we need exponential backoff though

Copy link
Member

@andybalaam andybalaam 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 - some questions

const backupInfo = await requestKeyBackupVersion(http);
let backupCallDone = false;
let backupInfo: KeyBackupInfo | null = null;
while (!backupCallDone) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if we are offline or something like that?

Copy link
Member

Choose a reason for hiding this comment

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

I.e. should we give up eventually?

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed with @richvdh, we might as well block the application until it works.
If we want to give up we would need to catch such errors in LifeCycle#handleLoadSessionFailure and is probably part of a bigger task for proper support of offline mode on web

Copy link
Member

Choose a reason for hiding this comment

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

Will you do this as part of this change, or separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not planned for now, needs design and some refactoring

src/rust-crypto/libolm_migration.ts Show resolved Hide resolved
@BillCarsonFr BillCarsonFr added this pull request to the merge queue Apr 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 5, 2024
@BillCarsonFr BillCarsonFr added this pull request to the merge queue Apr 8, 2024
Merged via the queue into develop with commit b352405 Apr 8, 2024
23 checks passed
@BillCarsonFr BillCarsonFr deleted the valere/element-r/fix_migration_error_with_backup branch April 8, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants