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

Tell system to reinitialize when it refuses to give us data we need #845

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

grote
Copy link
Collaborator

@grote grote commented Jan 10, 2025

Another hacky workaround to fix a hacky workaround.

We had talked to Google about the backup API not getting notified when essential K/V apps like @pm@ don't have new data for backup. The only option they offered is BackupMonitor which gets the information at least, but out of process on another thread.

So we implemented a hacky workaround where BackupMonitor tells SnapshotCreator to extract backup data from an old snapshot.

Unfortunately, it is possible that we do a backup run which includes @pm@, but encounters an error later, so the system cancels the entire backup which causes us not to have @pm@ data in a snapshots for re-use. Still, the system thinks we backed up @pm@ and doesn't give us its data.

Closes #818

@grote grote requested a review from t-m-w January 10, 2025 16:48
@grote
Copy link
Collaborator Author

grote commented Jan 10, 2025

Draft, only because based on #835.

One way to test this is to do a backup, then manually remove all existing snapshots from the backend. Then restart the Seedvault process and do another backup and watch the log for No latest snapshot! Initializing transport... and see if the backup succeeds.

Another hacky workaround to fix a hacky workaround.

We had talked to Google about the backup API not getting notified when essential K/V apps like @pm@ don't have new data for backup. The only option they offered is BackupMonitor which gets the information at least, but out of process on another thread.

 So we implemented a hacky workaround where BackupMonitor tells SnapshotCreator to extract backup data from an old snapshot.

 Unfortunately, it is possible that we do a backup run which includes @pm@, but encounters an error later, so the system cancels the entire backup which causes us not to have @pm@ data in a snapshots for re-use. Still, the system thinks we backed up @pm@ and doesn't give us its data.
@grote grote force-pushed the no-data-no-snapshot branch from eaee625 to f4b32e4 Compare January 21, 2025 18:25
@grote grote mentioned this pull request Feb 4, 2025
@grote
Copy link
Collaborator Author

grote commented Feb 4, 2025

@t-m-w as lots of people seem to run into this, please let's prioritize this for merging.

@t-m-w
Copy link
Collaborator

t-m-w commented Feb 5, 2025

@t-m-w as lots of people seem to run into this, please let's prioritize this for merging.

@grote This testing looks straightforward, so I will try to get to it soon. That being said, given that it is so straightforward, it is a shame that it is not really possible for affected users or community volunteers to test things like this themselves if they wish, I assume largely due to the OS-dependent process of building, signing, and shipping Seedvault — or well, at least the signing part. (I mean, it's not trivially possible for us to do, either, for those same reasons.) This is probably not relevant discussion for this PR, but just noting it.

@t-m-w
Copy link
Collaborator

t-m-w commented Feb 5, 2025

I've been running this on my daily driver for at least a week or two without issue, but I am just now getting around to testing.

One way to test this is to do a backup, then manually remove all existing snapshots from the backend. Then restart the Seedvault process and do another backup and watch the log for No latest snapshot! Initializing transport... and see if the backup succeeds.

I tried this, and indeed I received:

E c.s.s.r.SnapshotCreator: [binder:26015_B      ] No latest snapshot! Initializing transport...

The snapshot appeared to succeed, and I see it when I try to restore. @grote Anything else you'd like me to try?

Copy link
Collaborator

@t-m-w t-m-w left a comment

Choose a reason for hiding this comment

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

Approving based on the suggested test working as expected and not encountering any other issues while having the change on device for a while

@grote
Copy link
Collaborator Author

grote commented Feb 6, 2025

Awesome, thanks for testing!

@grote grote merged commit c56addb into seedvault-app:android15 Feb 6, 2025
9 checks passed
@grote grote deleted the no-data-no-snapshot branch February 6, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error finishing backup: IllegalStateException: No metadata for @pm@
2 participants