-
Notifications
You must be signed in to change notification settings - Fork 75
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
Verify app backup integrity #785
base: android15
Are you sure you want to change the base?
Conversation
I've been testing this with a USB drive. If the drive is removed during the integrity check, the error is: Some bits from the log (let me know if you want more):
I don't see it as a blocker for this change at all, but in the future, it would be nice to be able to perform operations that we know will never attempt to modify a repository even when something fails or is missing, and even if that's just a directory. I can create an issue for that if you want. |
My guess is that, for a newly-restored device, this feature is only able to check the integrity of preexisting backups if "Reuse" was chosen after Restore. Is that right? I actually did not choose "Create new backup" or "Reuse" - I got out of the choice by simply closing Seedvault, since at the moment I wasn't wanting to alter the backups at all and wasn't yet intending to turn on "Back up" of anything - so it would be whatever is the default behavior in that strange scenario. Currently, if I go to Restore, I see 4 snapshots available. After checking them, the verification screen shows that it only found and checked one. It won't be super common, but it could happen that users may wonder why the verification doesn't check all of the snapshots that they can see in Restore. I wonder how to explain that. |
I think I can answer my own question with "yes" based on further testing. Now I have a new question: If I only see three snapshots in Restore, and then I do an integrity check for 100% of it, then shouldn't any file I alter in .SeedVaultAndroidBackup be detected in some way? Especially if the check reports that they were all fine? That's where this all gets tricky for me to tell if it's working. I've sometimes needed to alter multiple files before anything is noticed and I'm not sure why. Most recently I chose to edit |
This is actually a feature, not a bug ;) I agree it would be nice not to modify anything for certain operations. However, how the backends now work is to create folders only when they are needed and not only when the backup is initialized. This simplified the design and eliminated a class of bugs we used to have. Also, creating directories should be totally harmless and not interfere with the backup. What may be happening with the flash drive is that it confuses the exception that gets thrown for the missing drive to mean the the directory is missing and then tries to create it.
Yes, if you don't re-use, there's normally no backups to check on your backend as a new recovery code gets created.
That does sound like a bug (unrelated to this MR). Could you try to reproduce please and then open a new ticket with the steps? The expected behavior is that you get asked again until you make a decision.
Did you do a backup before using the check feature? There's one from 45min ago, this may be it? So this would explain why it can only check that one snapshot while there's others (bound to other keys/codes) on your storage drive. But then I wonder why it even offers those backups for restore. Could it be that those are v1 backups?
Yes, if it doesn't find an issue even though you are sure you corrupted an essential file (in the right folder being used for backups), then I need to logs of that check run to debug.
yeah that isn't the right thing to mess with. |
Filed here: #789
Yes.
This is part of a test set I have been calling "v2", so I don't think so, but it's possible I guess. I can't remember. How would I tell? (Edit: They're definitely not all v1, but it's possible at least one of them is.)
That explains that! I will do a little more testing with files in other folders, then. |
Thanks!
Ok, so the ticket above seems to suggest that you already restored a previous backup with this Seedvault installation. So it did import the old key then. This explains why it can offer the old backups for restore. However, it didn't do the re-use thing, so it created a new backup repository for new backups where it does its checks in. |
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.
Tested by:
- Altering a single byte in a file (within a sha256 hash app-backups folder)
- Removing a single file (within a sha256 hash app-backups folder)
It detected issues in either scenario. I think this is good.
and error notification
fedeb15
to
c01cf4c
Compare
c01cf4c
to
6d9c18b
Compare
@t-m-w thanks for your testing! Before merging, could you please re-test the latest version of this MR by
Then seeing if the check catches those modifications and re-trying after doing a new backup. The expectation is that the new backup is fine, but that old backups still show errors. |
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.
Adding, removing, or altering the bytes of a file all results in a failed integrity check as expected. On performing a new backup, the preexisting snapshot continues to fail the check, while the new one does not. Further alterations can be made to cause corruption in the new snapshot; in the same way, running another backup will result in a new uncorrupted snapshot while the prior two remain corrupted.
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.
With the latest commit, the restoration process proceeds 1 minute after a hash failure is detected (the timeout). While it would be nice for it to proceed immediately upon determining this, I think this is mostly a system limitation. Seems acceptable enough; at least it proceeds now, and continues successfully (at least with the single-file corruption I made).
11-14 13:24:54.660 10174 3404 3651 W FullRestore: Error copying stream for package org.fdroid.basic.
[...]
11-14 13:25:53.252 1000 1338 2649 D BackupManagerService: Timeout message received for token=5c4a340b
11-14 13:25:53.253 1000 1338 2649 V LifecycleOperationStorage: [UserID:0] Cancel: token=5c4a340b
11-14 13:25:53.254 1000 1338 2649 W StreamFeederThread: Full-data restore target timed out; shutting down
11-14 13:25:53.258 1000 1338 3944 W BackupManagerService: Agent failure restoring org.fdroid.basic; ending restore
11-14 13:25:53.258 10174 3404 3651 D com.stevesoltys.seedvault.BackupMonitor: org.fdroid.basic cat: 3 id: 80
11-14 13:25:53.259 10174 3404 3651 D com.stevesoltys.seedvault.BackupMonitor: org.fdroid.basic cat: 2 id: 45
11-14 13:25:53.260 10174 3404 3650 D RestoreCoordinator: abortFullRestore
11-14 13:25:53.260 10209 3958 3980 W FullBackup: Incomplete read: expected 24825271 but got 36573769
11-14 13:25:53.260 10174 3404 3650 I FullRestore: Abort full restore of org.fdroid.basic!
11-14 13:25:53.261 10174 3404 3650 I FullRestore: Resetting state.
Adds a new option to app backup section in settings which brings up a new screen for checking app backups. User can define how large the random sample to be checked shall be, from 5% to 100%. Then checking happens in the background reporting progress in a notification. A final notification gets shown when the process finished when brings up a detailed report about the issues found, if any. Usually, a success screen is shown.
Here the error case:
Screen_recording_20241030_100936.mp4
Fixes #290, #798