Skip to content

Restore backup never reports load_keys progress of empty backup #4703

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

Closed
ajbura opened this issue Feb 9, 2025 · 1 comment · Fixed by #4711
Closed

Restore backup never reports load_keys progress of empty backup #4703

ajbura opened this issue Feb 9, 2025 · 1 comment · Fixed by #4711
Labels
A-Element-R Issues affecting the port of Element's crypto layer to Rust O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect

Comments

@ajbura
Copy link
Contributor

ajbura commented Feb 9, 2025

Before restoreKeyBackup starts the process of importing key backup, it reports load_keys without total, successes and failures as progress, indicating the progress is shifted from fetch_keys.

After that it reports progress with keys import data which client can use to track key restore progress, but in the case where backup doesn't have any keys (0 keys) it doesn't report with

opts?.progressCallback?.({
    total: 0,
    successes: 0,
    stage: "load_keys",
    failures: 0,
});

and tricks the client thinking forever that import progress is yet to happen

this should contain actual total number of keys with successes and failures set to 0

opts?.progressCallback?.({
stage: "load_keys",
});

@dosubot dosubot bot added A-Element-R Issues affecting the port of Element's crypto layer to Rust T-Defect labels Feb 9, 2025
@richvdh richvdh changed the title Restore backup never report load_keys progress of empty backup Restore backup never reports load_keys progress of empty backup Feb 10, 2025
@richvdh
Copy link
Member

richvdh commented Feb 10, 2025

The documentation on ImportRoomKeyProgressData is poor, but for the record, I believe the intended flow is:

  1. {stage: "fetch"}: it is downloading the keys
  2. {stage: "load_keys"}: it is starting to import the keys
  3. {stage: "load_keys", total: X, successes: Y, failures: Z }: import is proceeding or complete.

I agree, it looks like there is no final callback in the case of an empty backup.

this should contain actual total number of keys with successes and failures set to 0

Yes, that would seem sensible, though note that the call would need to be moved into importKeyBackup so as to have access to totalKeyCount. Would you make a PR to improve this?

It might also be good to have another stage which reports completion of the import, so that the client doesn't need to check the key counts. If that would be useful to you, feel free to make another PR.

@dbkr dbkr added S-Minor Impairs non-critical functionality or suitable workarounds exist O-Occasional Affects or can be seen by some users regularly or most users rarely O-Uncommon Most users are unlikely to come across this or unexpected workflow and removed O-Occasional Affects or can be seen by some users regularly or most users rarely labels Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Element-R Issues affecting the port of Element's crypto layer to Rust O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants