diff --git a/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt b/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt index 6d59857f..9640da50 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt @@ -9,14 +9,12 @@ import androidx.annotation.UiThread import androidx.annotation.WorkerThread import androidx.documentfile.provider.DocumentFile import androidx.preference.PreferenceManager -import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER import com.stevesoltys.seedvault.permitDiskReads import com.stevesoltys.seedvault.transport.backup.BackupCoordinator import java.util.concurrent.ConcurrentSkipListSet internal const val PREF_KEY_TOKEN = "token" 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_NAME = "storageName" @@ -126,15 +124,6 @@ class SettingsManager(private val context: 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 { return prefs.getBoolean(PREF_KEY_BACKUP_APK, true) } diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt index a7253f2c..d9294d05 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt @@ -43,16 +43,14 @@ private val TAG = BackupCoordinator::class.java.simpleName private class CoordinatorState( var calledInitialize: Boolean, var calledClearBackupData: Boolean, - var skippedPmBackup: Boolean, var cancelReason: PackageState ) { val expectFinish: Boolean - get() = calledInitialize || calledClearBackupData || skippedPmBackup + get() = calledInitialize || calledClearBackupData fun onFinish() { calledInitialize = false calledClearBackupData = false - skippedPmBackup = false cancelReason = UNKNOWN_ERROR } } @@ -79,7 +77,6 @@ internal class BackupCoordinator( private val state = CoordinatorState( calledInitialize = false, calledClearBackupData = false, - skippedPmBackup = false, cancelReason = UNKNOWN_ERROR ) @@ -236,38 +233,16 @@ internal class BackupCoordinator( flags: Int ): Int { state.cancelReason = UNKNOWN_ERROR - val packageName = packageInfo.packageName - // K/V backups (typically starting with package manager metadata - @pm@) - // are scheduled with JobInfo.Builder#setOverrideDeadline() and thus do not respect backoff. - // We need to reject them manually when we can not do a backup now. - // What else we tried can be found in: https://github.com/seedvault-app/seedvault/issues/102 - if (packageName == MAGIC_PACKAGE_MANAGER) { - val isIncremental = flags and FLAG_INCREMENTAL != 0 - 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 + 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 } val token = settingsManager.getToken() ?: error("no token in performFullBackup") val salt = metadataManager.salt @@ -388,14 +363,26 @@ internal class BackupCoordinator( check(!full.hasState()) { "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()!! - onPackageBackedUp(packageInfo, BackupType.KV) - val isPmBackup = packageInfo.packageName == MAGIC_PACKAGE_MANAGER - val result = kv.finishBackup() - // hook in here to back up APKs of apps that are otherwise not allowed for backup - if (result == TRANSPORT_OK && isPmBackup) { - backUpApksOfNotBackedUpPackages() + val packageName = packageInfo.packageName + // tell K/V backup to finish + var result = kv.finishBackup() + if (result == TRANSPORT_OK) { + val isPmBackup = packageName == MAGIC_PACKAGE_MANAGER + // 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 } @@ -404,8 +391,17 @@ internal class BackupCoordinator( "Full backup has state, but K/V backup has dangling state as well" } // getCurrentPackage() not-null because we have state - onPackageBackedUp(full.getCurrentPackage()!!, BackupType.FULL) - full.finishBackup() + val packageInfo = full.getCurrentPackage()!! + 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.onFinish() @@ -472,14 +468,8 @@ internal class BackupCoordinator( } private suspend fun onPackageBackedUp(packageInfo: PackageInfo, type: BackupType) { - try { - plugin.getMetadataOutputStream().use { - 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 + plugin.getMetadataOutputStream().use { + metadataManager.onPackageBackedUp(packageInfo, type, it) } } diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt index 50e5ed7a..861a57ce 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/KVBackup.kt @@ -9,6 +9,7 @@ import android.app.backup.BackupTransport.TRANSPORT_OK import android.content.pm.PackageInfo import android.os.ParcelFileDescriptor import android.util.Log +import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER import com.stevesoltys.seedvault.crypto.Crypto import com.stevesoltys.seedvault.header.VERSION import com.stevesoltys.seedvault.header.getADForKV @@ -126,7 +127,18 @@ internal class KVBackup( Log.e(TAG, "Exception reading backup input", result.exception) 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 if (op.value == null) { Log.e(TAG, "Deleting record with key ${op.key}") @@ -190,10 +202,11 @@ internal class KVBackup( suspend fun finishBackup(): Int { val state = this.state ?: error("No state in finishBackup") 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 { if (state.needsUpload) uploadDb(state.token, state.name, packageName, state.db) + else state.db.close() TRANSPORT_OK } catch (e: IOException) { TRANSPORT_ERROR diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt index b7ee1c08..824230e8 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt @@ -121,6 +121,7 @@ internal class CoordinatorIntegrationTest : TransportTest() { val value2 = CapturingSlot() val bOutputStream = ByteArrayOutputStream() + every { metadataManager.requiresInit } returns false every { settingsManager.getToken() } returns token every { metadataManager.salt } returns salt // 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 bOutputStream = ByteArrayOutputStream() + every { metadataManager.requiresInit } returns false every { settingsManager.getToken() } returns token every { metadataManager.salt } returns salt // read one key/value record and write it to output stream diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt index 6d4aa383..9a58f3fb 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt @@ -1,8 +1,6 @@ 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_NON_INCREMENTAL_BACKUP_REQUIRED import android.app.backup.BackupTransport.TRANSPORT_NOT_INITIALIZED import android.app.backup.BackupTransport.TRANSPORT_OK import android.app.backup.BackupTransport.TRANSPORT_PACKAGE_REJECTED @@ -112,9 +110,12 @@ internal class BackupCoordinatorTest : BackupTest() { @Test fun `error notification when device initialization fails`() = runBlocking { + val maybeTrue = Random.nextBoolean() + every { settingsManager.getToken() } returns token 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 assertEquals(TRANSPORT_ERROR, backup.initializeDevice()) @@ -132,6 +133,7 @@ internal class BackupCoordinatorTest : BackupTest() { runBlocking { every { settingsManager.getToken() } returns token coEvery { plugin.initializeDevice() } throws IOException() + every { metadataManager.requiresInit } returns false every { settingsManager.canDoBackupNow() } returns false 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 fun `performIncrementalBackup of @pm@ causes re-init when legacy format`() = runBlocking { val packageInfo = PackageInfo().apply { packageName = MAGIC_PACKAGE_MANAGER } @@ -255,24 +234,34 @@ internal class BackupCoordinatorTest : BackupTest() { @Test fun `finish backup delegates to KV plugin if it has state`() = runBlocking { - val result = Random.nextInt() - every { kv.hasState() } returns true every { full.hasState() } returns false every { kv.getCurrentPackage() } returns packageInfo + coEvery { kv.finishBackup() } returns TRANSPORT_OK every { settingsManager.getToken() } returns token coEvery { plugin.getOutputStream(token, FILE_BACKUP_METADATA) } returns metadataOutputStream every { metadataManager.onPackageBackedUp(packageInfo, BackupType.KV, metadataOutputStream) } just Runs - coEvery { kv.finishBackup() } returns result every { metadataOutputStream.close() } just Runs - assertEquals(result, backup.finishBackup()) + assertEquals(TRANSPORT_OK, backup.finishBackup()) 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 fun `finish backup delegates to full plugin if it has state`() = runBlocking { val result = Random.nextInt() @@ -280,12 +269,12 @@ internal class BackupCoordinatorTest : BackupTest() { every { kv.hasState() } returns false every { full.hasState() } returns true every { full.getCurrentPackage() } returns packageInfo + every { full.finishBackup() } returns result every { settingsManager.getToken() } returns token coEvery { plugin.getOutputStream(token, FILE_BACKUP_METADATA) } returns metadataOutputStream every { metadataManager.onPackageBackedUp(packageInfo, BackupType.FULL, metadataOutputStream) } just Runs - every { full.finishBackup() } returns result every { metadataOutputStream.close() } just Runs assertEquals(result, backup.finishBackup()) diff --git a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt index 83ce4ec7..e370f080 100644 --- a/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt +++ b/app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt @@ -16,6 +16,7 @@ import com.stevesoltys.seedvault.plugins.StoragePlugin import io.mockk.CapturingSlot import io.mockk.Runs import io.mockk.coEvery +import io.mockk.coVerify import io.mockk.every import io.mockk.just import io.mockk.mockk @@ -116,6 +117,7 @@ internal class KVBackupTest : BackupTest() { assertTrue(backup.hasState()) verify { data.close() } + every { db.close() } just Runs assertEquals(TRANSPORT_OK, backup.finishBackup()) assertFalse(backup.hasState()) @@ -153,6 +155,9 @@ internal class KVBackupTest : BackupTest() { assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, 0, token, salt)) assertTrue(backup.hasState()) + + every { db.close() } just Runs + assertEquals(TRANSPORT_OK, backup.finishBackup()) 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) { initPlugin(hasDataForPackage) every { db.put(key, dataValue) } just Runs