-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fixes to the backup import #5086
Conversation
f3effee
to
71f8626
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level looks fine. I didn't review the code detailed though, if you'd like me to I can on Monday when I have a full-size computer for it
6364bde
to
5fe48a1
Compare
1abb12e
to
2af9ff1
Compare
f75fbef
to
981c388
Compare
8874b8b
to
d5921cd
Compare
I checked with |
d5921cd
to
747fb9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the first commit message:
If a db wasn't imported successfully, unpacked blobs aren't deleted because we don't know at which step the import failed and whether the db will reference the blobs after a restart.
Why do we care about existing blobs? Backup is always imported into unconfigured account that we don't care about, it is fine to completely remove its blobs and database.
It should be safe to import backup like this:
- Remove database.
- Remove all blobs from blobdir.
- Unpack the .tar contents into blobdir.
- Move the new database over.
If any step after removing the database fails, we will have no database and create a new one next time.
To simplify further, there is no need to even remove existing blobs, if they exist they are non-sensitive generic images like device chat avatar, it will be garbage-collected anyway.
It seems the only change needed for the fix is to move the code that calls fs::rename
(or sql.import
) on the unpacked database to the end of the unpack loop. Trying to remove created garbage on failure is also somewhat helpful but cannot be done reliably anyway as the app may be killed anytime in progress: in the end account manager should care to delete the account folder before removing it from accounts.toml.
If the account manager isn't used, e.g. in deltachat-repl, while we import a backup into an unconfigured account as well, we mustn't remove already unpacked blobs if the db itself has been potentially imported, otherwise it may reference removed blobs. The user may forget to check the import result and try to use the inconsistent account. Maybe this is indeed unimportant and we can always remove blobs in fail scenario. The first commit is about fixing #4307 in most cases so that if there's insufficient disk space we do the cleanup asap on failure, not on the next program run or so. Though it's still good to clean up half-imported accounts after restart f.e. Also this commit makes things happen in a well-defined order: first unpack everything, then import the db. |
747fb9b
to
edf2884
Compare
In my last message I propose to only move unpacked database over as the last step. Worst case this step fails and user is left without the database at all and blobdir full of files that we can try to remove but that will be garbage-collected anyway at the next housekeeping with a new empty database. |
But this is exactly what is done in the first commit. I only added blobs removal also so that if we fail because of insufficient disk space, we don't worsen this situation. I only don't remove blobs after a try to import the db. Even if the db is imported with simple |
This way we can't get an account with missing blobs if there's not enough disk space. Also delete already unpacked files if all files weren't unpacked successfully. Still, there are some minor problems remaining: - If a db wasn't imported successfully, unpacked blobs aren't deleted because we don't know at which step the import failed and whether the db will reference the blobs after restart. - If `delete_and_reset_all_device_msgs()` fails, the whole `import_backup()` fails also, but after a restart delete_and_reset_all_device_msgs() isn't retried. Probably errors from it should be ignored at all.
Fix the progress calculation, before `total_size.checked_div(file_size)` was giving 0 if `total_size < file_size`.
Otherwise we continue to work with an incompletely imported db... but only until restart -- after that all changes to the db are lost.
…ice_msgs() They are not a good reason to fail the whole import. Anyway `delete_and_reset_all_device_msgs()` isn't retried after restarting the program.
edf2884
to
07f3c22
Compare
See commit messages.
No tests, i only checked this manually with
deltachat-repl
.