Improve behavior of @pm@ backups when we can't do backups

Before, we were faking a backup and just returned true, but remembering that next time, we need to do a fresh non-incremental @pm@ backup.
Now, we backup to local cache, but don't upload it. On next run, when we can do backups again, we will upload the updated cache. This simplifies things and reduces the special logic required.
This commit is contained in:
Torsten Grote 2021-10-06 20:44:48 +02:00 committed by Chirayu Desai
parent 36c35d6f98
commit b029b0b029
6 changed files with 107 additions and 96 deletions

View file

@ -9,14 +9,12 @@ import androidx.annotation.UiThread
import androidx.annotation.WorkerThread import androidx.annotation.WorkerThread
import androidx.documentfile.provider.DocumentFile import androidx.documentfile.provider.DocumentFile
import androidx.preference.PreferenceManager import androidx.preference.PreferenceManager
import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER
import com.stevesoltys.seedvault.permitDiskReads import com.stevesoltys.seedvault.permitDiskReads
import com.stevesoltys.seedvault.transport.backup.BackupCoordinator import com.stevesoltys.seedvault.transport.backup.BackupCoordinator
import java.util.concurrent.ConcurrentSkipListSet import java.util.concurrent.ConcurrentSkipListSet
internal const val PREF_KEY_TOKEN = "token" internal const val PREF_KEY_TOKEN = "token"
internal const val PREF_KEY_BACKUP_APK = "backup_apk" internal const val PREF_KEY_BACKUP_APK = "backup_apk"
internal const val PREF_KEY_REDO_PM = "redoPm"
private const val PREF_KEY_STORAGE_URI = "storageUri" private const val PREF_KEY_STORAGE_URI = "storageUri"
private const val PREF_KEY_STORAGE_NAME = "storageName" private const val PREF_KEY_STORAGE_NAME = "storageName"
@ -126,15 +124,6 @@ class SettingsManager(private val context: Context) {
return !storage.isUnavailableUsb(context) && !storage.isUnavailableNetwork(context) return !storage.isUnavailableUsb(context) && !storage.isUnavailableNetwork(context)
} }
/**
* Set this to true if the next backup run for [MAGIC_PACKAGE_MANAGER]
* needs to be non-incremental,
* because we need to fake an OK backup now even though we can't do one right now.
*/
var pmBackupNextTimeNonIncremental: Boolean
get() = prefs.getBoolean(PREF_KEY_REDO_PM, false)
set(value) = prefs.edit().putBoolean(PREF_KEY_REDO_PM, value).apply()
fun backupApks(): Boolean { fun backupApks(): Boolean {
return prefs.getBoolean(PREF_KEY_BACKUP_APK, true) return prefs.getBoolean(PREF_KEY_BACKUP_APK, true)
} }

View file

@ -43,16 +43,14 @@ private val TAG = BackupCoordinator::class.java.simpleName
private class CoordinatorState( private class CoordinatorState(
var calledInitialize: Boolean, var calledInitialize: Boolean,
var calledClearBackupData: Boolean, var calledClearBackupData: Boolean,
var skippedPmBackup: Boolean,
var cancelReason: PackageState var cancelReason: PackageState
) { ) {
val expectFinish: Boolean val expectFinish: Boolean
get() = calledInitialize || calledClearBackupData || skippedPmBackup get() = calledInitialize || calledClearBackupData
fun onFinish() { fun onFinish() {
calledInitialize = false calledInitialize = false
calledClearBackupData = false calledClearBackupData = false
skippedPmBackup = false
cancelReason = UNKNOWN_ERROR cancelReason = UNKNOWN_ERROR
} }
} }
@ -79,7 +77,6 @@ internal class BackupCoordinator(
private val state = CoordinatorState( private val state = CoordinatorState(
calledInitialize = false, calledInitialize = false,
calledClearBackupData = false, calledClearBackupData = false,
skippedPmBackup = false,
cancelReason = UNKNOWN_ERROR cancelReason = UNKNOWN_ERROR
) )
@ -236,38 +233,16 @@ internal class BackupCoordinator(
flags: Int flags: Int
): Int { ): Int {
state.cancelReason = UNKNOWN_ERROR state.cancelReason = UNKNOWN_ERROR
val packageName = packageInfo.packageName if (metadataManager.requiresInit) {
// K/V backups (typically starting with package manager metadata - @pm@) // start a new restore set to upgrade from legacy format
// are scheduled with JobInfo.Builder#setOverrideDeadline() and thus do not respect backoff. // by starting a clean backup with all files using the new version
// We need to reject them manually when we can not do a backup now. try {
// What else we tried can be found in: https://github.com/seedvault-app/seedvault/issues/102 startNewRestoreSet()
if (packageName == MAGIC_PACKAGE_MANAGER) { } catch (e: IOException) {
val isIncremental = flags and FLAG_INCREMENTAL != 0 Log.e(TAG, "Error starting new restore set", e)
if (metadataManager.requiresInit) {
// start a new restore set to upgrade from legacy format
// by starting a clean backup with all files using the new version
try {
startNewRestoreSet()
} catch (e: IOException) {
Log.e(TAG, "Error starting new restore set", e)
}
// this causes a backup error, but things should go back to normal afterwards
return TRANSPORT_NOT_INITIALIZED
} else if (!settingsManager.canDoBackupNow()) {
// Returning anything else here (except non-incremental-required which re-tries)
// will make the system consider the backup state compromised
// and force re-initialization on next run.
// Errors for other packages are OK, but this one is not allowed to fail.
Log.w(TAG, "Skipping @pm@ backup as we can't do backup right now.")
state.skippedPmBackup = true
settingsManager.pmBackupNextTimeNonIncremental = true
data.close()
return TRANSPORT_OK
} else if (isIncremental && settingsManager.pmBackupNextTimeNonIncremental) {
settingsManager.pmBackupNextTimeNonIncremental = false
data.close()
return TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED
} }
// this causes a backup error, but things should go back to normal afterwards
return TRANSPORT_NOT_INITIALIZED
} }
val token = settingsManager.getToken() ?: error("no token in performFullBackup") val token = settingsManager.getToken() ?: error("no token in performFullBackup")
val salt = metadataManager.salt val salt = metadataManager.salt
@ -388,14 +363,26 @@ internal class BackupCoordinator(
check(!full.hasState()) { check(!full.hasState()) {
"K/V backup has state, but full backup has dangling state as well" "K/V backup has state, but full backup has dangling state as well"
} }
// getCurrentPackage() not-null because we have state // getCurrentPackage() not-null because we have state, call before finishing
val packageInfo = kv.getCurrentPackage()!! val packageInfo = kv.getCurrentPackage()!!
onPackageBackedUp(packageInfo, BackupType.KV) val packageName = packageInfo.packageName
val isPmBackup = packageInfo.packageName == MAGIC_PACKAGE_MANAGER // tell K/V backup to finish
val result = kv.finishBackup() var result = kv.finishBackup()
// hook in here to back up APKs of apps that are otherwise not allowed for backup if (result == TRANSPORT_OK) {
if (result == TRANSPORT_OK && isPmBackup) { val isPmBackup = packageName == MAGIC_PACKAGE_MANAGER
backUpApksOfNotBackedUpPackages() // call onPackageBackedUp for @pm@ only if we can do backups right now
if (!isPmBackup || settingsManager.canDoBackupNow()) {
try {
onPackageBackedUp(packageInfo, BackupType.KV)
} catch (e: Exception) {
Log.e(TAG, "Error calling onPackageBackedUp for $packageName", e)
result = TRANSPORT_PACKAGE_REJECTED
}
}
// hook in here to back up APKs of apps that are otherwise not allowed for backup
if (isPmBackup && settingsManager.canDoBackupNow()) {
backUpApksOfNotBackedUpPackages()
}
} }
result result
} }
@ -404,8 +391,17 @@ internal class BackupCoordinator(
"Full backup has state, but K/V backup has dangling state as well" "Full backup has state, but K/V backup has dangling state as well"
} }
// getCurrentPackage() not-null because we have state // getCurrentPackage() not-null because we have state
onPackageBackedUp(full.getCurrentPackage()!!, BackupType.FULL) val packageInfo = full.getCurrentPackage()!!
full.finishBackup() val packageName = packageInfo.packageName
// tell full backup to finish
var result = full.finishBackup()
try {
onPackageBackedUp(packageInfo, BackupType.FULL)
} catch (e: Exception) {
Log.e(TAG, "Error calling onPackageBackedUp for $packageName", e)
result = TRANSPORT_PACKAGE_REJECTED
}
result
} }
state.expectFinish -> { state.expectFinish -> {
state.onFinish() state.onFinish()
@ -472,14 +468,8 @@ internal class BackupCoordinator(
} }
private suspend fun onPackageBackedUp(packageInfo: PackageInfo, type: BackupType) { private suspend fun onPackageBackedUp(packageInfo: PackageInfo, type: BackupType) {
try { plugin.getMetadataOutputStream().use {
plugin.getMetadataOutputStream().use { metadataManager.onPackageBackedUp(packageInfo, type, it)
metadataManager.onPackageBackedUp(packageInfo, type, it)
}
} catch (e: IOException) {
Log.e(TAG, "Error while writing metadata for ${packageInfo.packageName}", e)
// we are not re-throwing this as there's nothing we can do now
// except hoping the current metadata gets written with the next package
} }
} }

View file

@ -9,6 +9,7 @@ import android.app.backup.BackupTransport.TRANSPORT_OK
import android.content.pm.PackageInfo import android.content.pm.PackageInfo
import android.os.ParcelFileDescriptor import android.os.ParcelFileDescriptor
import android.util.Log import android.util.Log
import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER
import com.stevesoltys.seedvault.crypto.Crypto import com.stevesoltys.seedvault.crypto.Crypto
import com.stevesoltys.seedvault.header.VERSION import com.stevesoltys.seedvault.header.VERSION
import com.stevesoltys.seedvault.header.getADForKV import com.stevesoltys.seedvault.header.getADForKV
@ -126,7 +127,18 @@ internal class KVBackup(
Log.e(TAG, "Exception reading backup input", result.exception) Log.e(TAG, "Exception reading backup input", result.exception)
return backupError(TRANSPORT_ERROR) return backupError(TRANSPORT_ERROR)
} }
state.needsUpload = true state.needsUpload = if (state.packageInfo.packageName == MAGIC_PACKAGE_MANAGER) {
// Don't upload, if we currently can't do backups.
// If we tried, we would fail @pm@ backup which causes the system to do a re-init.
// See: https://github.com/seedvault-app/seedvault/issues/102
// K/V backups (typically starting with package manager metadata - @pm@)
// are scheduled with JobInfo.Builder#setOverrideDeadline()
// and thus do not respect backoff.
settingsManager.canDoBackupNow()
} else {
// all other packages always need upload
true
}
val op = (result as Result.Ok).result val op = (result as Result.Ok).result
if (op.value == null) { if (op.value == null) {
Log.e(TAG, "Deleting record with key ${op.key}") Log.e(TAG, "Deleting record with key ${op.key}")
@ -190,10 +202,11 @@ internal class KVBackup(
suspend fun finishBackup(): Int { suspend fun finishBackup(): Int {
val state = this.state ?: error("No state in finishBackup") val state = this.state ?: error("No state in finishBackup")
val packageName = state.packageInfo.packageName val packageName = state.packageInfo.packageName
Log.i(TAG, "Finish K/V Backup of $packageName") Log.i(TAG, "Finish K/V Backup of $packageName - needs upload: ${state.needsUpload}")
return try { return try {
if (state.needsUpload) uploadDb(state.token, state.name, packageName, state.db) if (state.needsUpload) uploadDb(state.token, state.name, packageName, state.db)
else state.db.close()
TRANSPORT_OK TRANSPORT_OK
} catch (e: IOException) { } catch (e: IOException) {
TRANSPORT_ERROR TRANSPORT_ERROR

View file

@ -121,6 +121,7 @@ internal class CoordinatorIntegrationTest : TransportTest() {
val value2 = CapturingSlot<ByteArray>() val value2 = CapturingSlot<ByteArray>()
val bOutputStream = ByteArrayOutputStream() val bOutputStream = ByteArrayOutputStream()
every { metadataManager.requiresInit } returns false
every { settingsManager.getToken() } returns token every { settingsManager.getToken() } returns token
every { metadataManager.salt } returns salt every { metadataManager.salt } returns salt
// read one key/value record and write it to output stream // read one key/value record and write it to output stream
@ -197,6 +198,7 @@ internal class CoordinatorIntegrationTest : TransportTest() {
val appData = ByteArray(size).apply { Random.nextBytes(this) } val appData = ByteArray(size).apply { Random.nextBytes(this) }
val bOutputStream = ByteArrayOutputStream() val bOutputStream = ByteArrayOutputStream()
every { metadataManager.requiresInit } returns false
every { settingsManager.getToken() } returns token every { settingsManager.getToken() } returns token
every { metadataManager.salt } returns salt every { metadataManager.salt } returns salt
// read one key/value record and write it to output stream // read one key/value record and write it to output stream

View file

@ -1,8 +1,6 @@
package com.stevesoltys.seedvault.transport.backup package com.stevesoltys.seedvault.transport.backup
import android.app.backup.BackupTransport.FLAG_INCREMENTAL
import android.app.backup.BackupTransport.TRANSPORT_ERROR import android.app.backup.BackupTransport.TRANSPORT_ERROR
import android.app.backup.BackupTransport.TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED
import android.app.backup.BackupTransport.TRANSPORT_NOT_INITIALIZED import android.app.backup.BackupTransport.TRANSPORT_NOT_INITIALIZED
import android.app.backup.BackupTransport.TRANSPORT_OK import android.app.backup.BackupTransport.TRANSPORT_OK
import android.app.backup.BackupTransport.TRANSPORT_PACKAGE_REJECTED import android.app.backup.BackupTransport.TRANSPORT_PACKAGE_REJECTED
@ -112,9 +110,12 @@ internal class BackupCoordinatorTest : BackupTest() {
@Test @Test
fun `error notification when device initialization fails`() = runBlocking { fun `error notification when device initialization fails`() = runBlocking {
val maybeTrue = Random.nextBoolean()
every { settingsManager.getToken() } returns token every { settingsManager.getToken() } returns token
coEvery { plugin.initializeDevice() } throws IOException() coEvery { plugin.initializeDevice() } throws IOException()
every { settingsManager.canDoBackupNow() } returns true every { metadataManager.requiresInit } returns maybeTrue
every { settingsManager.canDoBackupNow() } returns !maybeTrue
every { notificationManager.onBackupError() } just Runs every { notificationManager.onBackupError() } just Runs
assertEquals(TRANSPORT_ERROR, backup.initializeDevice()) assertEquals(TRANSPORT_ERROR, backup.initializeDevice())
@ -132,6 +133,7 @@ internal class BackupCoordinatorTest : BackupTest() {
runBlocking { runBlocking {
every { settingsManager.getToken() } returns token every { settingsManager.getToken() } returns token
coEvery { plugin.initializeDevice() } throws IOException() coEvery { plugin.initializeDevice() } throws IOException()
every { metadataManager.requiresInit } returns false
every { settingsManager.canDoBackupNow() } returns false every { settingsManager.canDoBackupNow() } returns false
assertEquals(TRANSPORT_ERROR, backup.initializeDevice()) assertEquals(TRANSPORT_ERROR, backup.initializeDevice())
@ -144,29 +146,6 @@ internal class BackupCoordinatorTest : BackupTest() {
} }
} }
@Test
fun `performIncrementalBackup fakes @pm@ when no backup possible`() = runBlocking {
val packageInfo = PackageInfo().apply { packageName = MAGIC_PACKAGE_MANAGER }
every { settingsManager.canDoBackupNow() } returns false
every { settingsManager.pmBackupNextTimeNonIncremental = true } just Runs
every { data.close() } just Runs
// returns OK even though we can't do backups
assertEquals(TRANSPORT_OK, backup.performIncrementalBackup(packageInfo, data, 0))
every { settingsManager.canDoBackupNow() } returns true
every { metadataManager.requiresInit } returns false
every { settingsManager.pmBackupNextTimeNonIncremental } returns true
every { settingsManager.pmBackupNextTimeNonIncremental = false } just Runs
// now that we can do backups again, it requests a full non-incremental backup of @pm@
assertEquals(
TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED,
backup.performIncrementalBackup(packageInfo, data, FLAG_INCREMENTAL)
)
}
@Test @Test
fun `performIncrementalBackup of @pm@ causes re-init when legacy format`() = runBlocking { fun `performIncrementalBackup of @pm@ causes re-init when legacy format`() = runBlocking {
val packageInfo = PackageInfo().apply { packageName = MAGIC_PACKAGE_MANAGER } val packageInfo = PackageInfo().apply { packageName = MAGIC_PACKAGE_MANAGER }
@ -255,24 +234,34 @@ internal class BackupCoordinatorTest : BackupTest() {
@Test @Test
fun `finish backup delegates to KV plugin if it has state`() = runBlocking { fun `finish backup delegates to KV plugin if it has state`() = runBlocking {
val result = Random.nextInt()
every { kv.hasState() } returns true every { kv.hasState() } returns true
every { full.hasState() } returns false every { full.hasState() } returns false
every { kv.getCurrentPackage() } returns packageInfo every { kv.getCurrentPackage() } returns packageInfo
coEvery { kv.finishBackup() } returns TRANSPORT_OK
every { settingsManager.getToken() } returns token every { settingsManager.getToken() } returns token
coEvery { plugin.getOutputStream(token, FILE_BACKUP_METADATA) } returns metadataOutputStream coEvery { plugin.getOutputStream(token, FILE_BACKUP_METADATA) } returns metadataOutputStream
every { every {
metadataManager.onPackageBackedUp(packageInfo, BackupType.KV, metadataOutputStream) metadataManager.onPackageBackedUp(packageInfo, BackupType.KV, metadataOutputStream)
} just Runs } just Runs
coEvery { kv.finishBackup() } returns result
every { metadataOutputStream.close() } just Runs every { metadataOutputStream.close() } just Runs
assertEquals(result, backup.finishBackup()) assertEquals(TRANSPORT_OK, backup.finishBackup())
verify { metadataOutputStream.close() } verify { metadataOutputStream.close() }
} }
@Test
fun `finish backup does not upload @pm@ metadata, if it can't do backups`() = runBlocking {
every { kv.hasState() } returns true
every { full.hasState() } returns false
every { kv.getCurrentPackage() } returns pmPackageInfo
coEvery { kv.finishBackup() } returns TRANSPORT_OK
every { settingsManager.canDoBackupNow() } returns false
assertEquals(TRANSPORT_OK, backup.finishBackup())
}
@Test @Test
fun `finish backup delegates to full plugin if it has state`() = runBlocking { fun `finish backup delegates to full plugin if it has state`() = runBlocking {
val result = Random.nextInt() val result = Random.nextInt()
@ -280,12 +269,12 @@ internal class BackupCoordinatorTest : BackupTest() {
every { kv.hasState() } returns false every { kv.hasState() } returns false
every { full.hasState() } returns true every { full.hasState() } returns true
every { full.getCurrentPackage() } returns packageInfo every { full.getCurrentPackage() } returns packageInfo
every { full.finishBackup() } returns result
every { settingsManager.getToken() } returns token every { settingsManager.getToken() } returns token
coEvery { plugin.getOutputStream(token, FILE_BACKUP_METADATA) } returns metadataOutputStream coEvery { plugin.getOutputStream(token, FILE_BACKUP_METADATA) } returns metadataOutputStream
every { every {
metadataManager.onPackageBackedUp(packageInfo, BackupType.FULL, metadataOutputStream) metadataManager.onPackageBackedUp(packageInfo, BackupType.FULL, metadataOutputStream)
} just Runs } just Runs
every { full.finishBackup() } returns result
every { metadataOutputStream.close() } just Runs every { metadataOutputStream.close() } just Runs
assertEquals(result, backup.finishBackup()) assertEquals(result, backup.finishBackup())

View file

@ -16,6 +16,7 @@ import com.stevesoltys.seedvault.plugins.StoragePlugin
import io.mockk.CapturingSlot import io.mockk.CapturingSlot
import io.mockk.Runs import io.mockk.Runs
import io.mockk.coEvery import io.mockk.coEvery
import io.mockk.coVerify
import io.mockk.every import io.mockk.every
import io.mockk.just import io.mockk.just
import io.mockk.mockk import io.mockk.mockk
@ -116,6 +117,7 @@ internal class KVBackupTest : BackupTest() {
assertTrue(backup.hasState()) assertTrue(backup.hasState())
verify { data.close() } verify { data.close() }
every { db.close() } just Runs
assertEquals(TRANSPORT_OK, backup.finishBackup()) assertEquals(TRANSPORT_OK, backup.finishBackup())
assertFalse(backup.hasState()) assertFalse(backup.hasState())
@ -153,6 +155,9 @@ internal class KVBackupTest : BackupTest() {
assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, 0, token, salt)) assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, 0, token, salt))
assertTrue(backup.hasState()) assertTrue(backup.hasState())
every { db.close() } just Runs
assertEquals(TRANSPORT_OK, backup.finishBackup()) assertEquals(TRANSPORT_OK, backup.finishBackup())
assertFalse(backup.hasState()) assertFalse(backup.hasState())
} }
@ -221,6 +226,29 @@ internal class KVBackupTest : BackupTest() {
} }
} }
@Test
fun `no upload when we back up @pm@ while we can't do backups`() = runBlocking {
every { dbManager.existsDb(pmPackageInfo.packageName) } returns false
every { crypto.getNameForPackage(salt, pmPackageInfo.packageName) } returns name
every { dbManager.getDb(pmPackageInfo.packageName) } returns db
every { settingsManager.canDoBackupNow() } returns false
every { db.put(key, dataValue) } just Runs
getDataInput(listOf(true, false))
assertEquals(TRANSPORT_OK, backup.performBackup(pmPackageInfo, data, 0, token, salt))
assertTrue(backup.hasState())
assertEquals(pmPackageInfo, backup.getCurrentPackage())
every { db.close() } just Runs
assertEquals(TRANSPORT_OK, backup.finishBackup())
assertFalse(backup.hasState())
coVerify(exactly = 0) {
plugin.getOutputStream(token, name)
}
}
private fun singleRecordBackup(hasDataForPackage: Boolean = false) { private fun singleRecordBackup(hasDataForPackage: Boolean = false) {
initPlugin(hasDataForPackage) initPlugin(hasDataForPackage)
every { db.put(key, dataValue) } just Runs every { db.put(key, dataValue) } just Runs