diff --git a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsStorage.kt b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsStorage.kt index 309cc3c9..ddda2c73 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsStorage.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsStorage.kt @@ -177,11 +177,17 @@ internal suspend fun DocumentFile.createOrGetFile( name: String, mimeType: String = MIME_TYPE ): DocumentFile { - return findFileBlocking(context, name) ?: createFile(mimeType, name)?.apply { - if (this.name != name) { - throw IOException("File named ${this.name}, but should be $name") - } - } ?: throw IOException() + return try { + findFileBlocking(context, name) ?: createFile(mimeType, name)?.apply { + if (this.name != name) { + throw IOException("File named ${this.name}, but should be $name") + } + } ?: throw IOException() + } catch (e: IllegalArgumentException) { + // Can be thrown by FileSystemProvider#isChildDocument() when flash drive is not plugged-in + // http://aosp.opersys.com/xref/android-11.0.0_r8/xref/frameworks/base/core/java/com/android/internal/content/FileSystemProvider.java#135 + throw IOException(e) + } } /** diff --git a/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsFragment.kt b/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsFragment.kt index 3c9a90e2..6acf3476 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsFragment.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsFragment.kt @@ -225,16 +225,12 @@ class SettingsFragment : PreferenceFragmentCompat() { private suspend fun setMenuItemStates() { if (menuBackupNow != null && menuRestore != null) { - val storage = this.storage - val enabled = storage != null && storageAvailable(storage) + val enabled = withContext(Dispatchers.IO) { + settingsManager.canDoBackupNow() + } menuBackupNow?.isEnabled = enabled menuRestore?.isEnabled = enabled } } - private suspend fun storageAvailable(storage: Storage) = withContext(Dispatchers.IO) { - val context = context ?: return@withContext false - !storage.isUnavailableUsb(context) && !storage.isUnavailableNetwork(context) - } - } 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 6773eb33..0217638d 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/settings/SettingsManager.kt @@ -9,12 +9,14 @@ 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" @@ -28,7 +30,7 @@ private const val PREF_KEY_FLASH_DRIVE_PRODUCT_ID = "flashDriveProductId" private const val PREF_KEY_BACKUP_APP_BLACKLIST = "backupAppBlacklist" -class SettingsManager(context: Context) { +class SettingsManager(private val context: Context) { private val prefs = permitDiskReads { PreferenceManager.getDefaultSharedPreferences(context) @@ -107,6 +109,29 @@ class SettingsManager(context: Context) { return FlashDrive(name, serialNumber, vendorId, productId) } + /** + * Check if we are able to do backups now by examining possible pre-conditions + * such as plugged-in flash drive or internet access. + * + * Should be run off the UI thread (ideally I/O) because of disk access. + * + * @return true if a backup is possible, false if not. + */ + @WorkerThread + fun canDoBackupNow(): Boolean { + val storage = getStorage() ?: return false + 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 850dc869..bf48b8bb 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 @@ -36,6 +36,23 @@ import java.util.concurrent.TimeUnit.HOURS 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 + + fun onFinish() { + calledInitialize = false + calledClearBackupData = false + skippedPmBackup = false + cancelReason = UNKNOWN_ERROR + } +} + /** * @author Steve Soltys * @author Torsten Grote @@ -55,9 +72,12 @@ internal class BackupCoordinator( private val nm: BackupNotificationManager ) { - private var calledInitialize = false - private var calledClearBackupData = false - private var cancelReason: PackageState = UNKNOWN_ERROR + private val state = CoordinatorState( + calledInitialize = false, + calledClearBackupData = false, + skippedPmBackup = false, + cancelReason = UNKNOWN_ERROR + ) // ------------------------------------------------------------------------------------ // Transport initialization and quota @@ -108,12 +128,12 @@ internal class BackupCoordinator( } // [finishBackup] will only be called when we return [TRANSPORT_OK] here // so we remember that we initialized successfully - calledInitialize = true + state.calledInitialize = true TRANSPORT_OK } catch (e: IOException) { Log.e(TAG, "Error initializing device", e) // Show error notification if we were ready for backups - if (getBackupBackoff() == 0L) nm.onBackupError() + if (settingsManager.canDoBackupNow()) nm.onBackupError() TRANSPORT_ERROR } @@ -211,13 +231,30 @@ internal class BackupCoordinator( data: ParcelFileDescriptor, flags: Int ): Int { - cancelReason = UNKNOWN_ERROR + state.cancelReason = UNKNOWN_ERROR val packageName = packageInfo.packageName - // K/V backups (typically starting with package manager metadata) + // 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 now is not a good time for a backup. - if (packageName == MAGIC_PACKAGE_MANAGER && getBackupBackoff() != 0L) { - return TRANSPORT_PACKAGE_REJECTED + // 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/stevesoltys/seedvault/issues/102 + if (packageName == MAGIC_PACKAGE_MANAGER) { + 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 (flags and FLAG_INCREMENTAL != 0 && + settingsManager.pmBackupNextTimeNonIncremental + ) { + settingsManager.pmBackupNextTimeNonIncremental = false + data.close() + return TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED + } } val result = kv.performBackup(packageInfo, data, flags) if (result == TRANSPORT_OK && packageName == MAGIC_PACKAGE_MANAGER) { @@ -250,8 +287,8 @@ internal class BackupCoordinator( fun checkFullBackupSize(size: Long): Int { val result = full.checkFullBackupSize(size) - if (result == TRANSPORT_PACKAGE_REJECTED) cancelReason = NO_DATA - else if (result == TRANSPORT_QUOTA_EXCEEDED) cancelReason = QUOTA_EXCEEDED + if (result == TRANSPORT_PACKAGE_REJECTED) state.cancelReason = NO_DATA + else if (result == TRANSPORT_QUOTA_EXCEEDED) state.cancelReason = QUOTA_EXCEEDED return result } @@ -260,7 +297,7 @@ internal class BackupCoordinator( fileDescriptor: ParcelFileDescriptor, flags: Int ): Int { - cancelReason = UNKNOWN_ERROR + state.cancelReason = UNKNOWN_ERROR return full.performFullBackup(targetPackage, fileDescriptor, flags) } @@ -282,7 +319,10 @@ internal class BackupCoordinator( suspend fun cancelFullBackup() { val packageInfo = full.getCurrentPackage() ?: throw AssertionError("Cancelling full backup, but no current package") - Log.i(TAG, "Cancel full backup of ${packageInfo.packageName} because of $cancelReason") + Log.i( + TAG, "Cancel full backup of ${packageInfo.packageName}" + + " because of $state.cancelReason" + ) onPackageBackupError(packageInfo) full.cancelFullBackup() } @@ -313,13 +353,13 @@ internal class BackupCoordinator( Log.w(TAG, "Error clearing full backup data for $packageName", e) return TRANSPORT_ERROR } - calledClearBackupData = true + state.calledClearBackupData = true return TRANSPORT_OK } /** - * Finish sending application data to the backup destination. - * This must be called after [performIncrementalBackup], [performFullBackup], or [clearBackupData] + * Finish sending application data to the backup destination. This must be called + * after [performIncrementalBackup], [performFullBackup], or [clearBackupData] * to ensure that all data is sent and the operation properly finalized. * Only when this method returns true can a backup be assumed to have succeeded. * @@ -340,9 +380,8 @@ internal class BackupCoordinator( onPackageBackedUp(full.getCurrentPackage()!!) // not-null because we have state full.finishBackup() } - calledInitialize || calledClearBackupData -> { - calledInitialize = false - calledClearBackupData = false + state.expectFinish -> { + state.onFinish() TRANSPORT_OK } else -> throw IllegalStateException("Unexpected state in finishBackup()") @@ -405,23 +444,24 @@ internal class BackupCoordinator( } private suspend fun onPackageBackedUp(packageInfo: PackageInfo) { - val packageName = packageInfo.packageName try { plugin.getMetadataOutputStream().use { metadataManager.onPackageBackedUp(packageInfo, it) } } catch (e: IOException) { - Log.e(TAG, "Error while writing metadata for $packageName", e) + 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 } } private suspend fun onPackageBackupError(packageInfo: PackageInfo) { // don't bother with system apps that have no data - if (cancelReason == NO_DATA && packageInfo.isSystemApp()) return + if (state.cancelReason == NO_DATA && packageInfo.isSystemApp()) return val packageName = packageInfo.packageName try { plugin.getMetadataOutputStream().use { - metadataManager.onPackageBackupError(packageInfo, cancelReason, it) + metadataManager.onPackageBackupError(packageInfo, state.cancelReason, it) } } catch (e: IOException) { Log.e(TAG, "Error while writing metadata for $packageName", e) 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 be0734c9..2468f75f 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 @@ -71,7 +71,10 @@ internal class KVBackup( this.state = KVBackupState(packageInfo) // no need for backup when no data has changed - if (dataNotChanged) return TRANSPORT_OK + if (dataNotChanged) { + data.close() + return TRANSPORT_OK + } // check if we have existing data for the given package val hasDataForPackage = try { 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 e973b922..74960721 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,6 +1,8 @@ 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_OK import android.app.backup.BackupTransport.TRANSPORT_PACKAGE_REJECTED import android.app.backup.BackupTransport.TRANSPORT_QUOTA_EXCEEDED @@ -62,7 +64,9 @@ internal class BackupCoordinatorTest : BackupTest() { private val metadataOutputStream = mockk() private val fileDescriptor: ParcelFileDescriptor = mockk() private val packageMetadata: PackageMetadata = mockk() - private val storage = Storage(Uri.EMPTY, getRandomString(), + private val storage = Storage( + uri = Uri.EMPTY, + name = getRandomString(), isUsb = false, requiresNetwork = false ) @@ -106,7 +110,7 @@ internal class BackupCoordinatorTest : BackupTest() { fun `error notification when device initialization fails`() = runBlocking { every { settingsManager.getToken() } returns token coEvery { plugin.initializeDevice() } throws IOException() - every { settingsManager.getStorage() } returns storage + every { settingsManager.canDoBackupNow() } returns true every { notificationManager.onBackupError() } just Runs assertEquals(TRANSPORT_ERROR, backup.initializeDevice()) @@ -120,14 +124,11 @@ internal class BackupCoordinatorTest : BackupTest() { } @Test - fun `no error notification when device initialization fails on unplugged USB storage`() = + fun `no error notification when device initialization fails when no backup possible`() = runBlocking { - val storage = mockk() - every { settingsManager.getToken() } returns token coEvery { plugin.initializeDevice() } throws IOException() - every { settingsManager.getStorage() } returns storage - every { storage.isUnavailableUsb(context) } returns true + every { settingsManager.canDoBackupNow() } returns false assertEquals(TRANSPORT_ERROR, backup.initializeDevice()) @@ -139,6 +140,28 @@ 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 { 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 `getBackupQuota() delegates to right plugin`() = runBlocking { val isFullBackup = Random.nextBoolean() @@ -330,7 +353,7 @@ internal class BackupCoordinatorTest : BackupTest() { ) val packageMetadata: PackageMetadata = mockk() - every { settingsManager.getStorage() } returns storage // to check for removable storage + every { settingsManager.canDoBackupNow() } returns true // do actual @pm@ backup coEvery { kv.performBackup(packageInfo, fileDescriptor, 0) } returns TRANSPORT_OK // now check if we have opt-out apps that we need to back up APKs for 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 367484f2..1a7f8d80 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 @@ -153,9 +153,13 @@ internal class KVBackupTest : BackupTest() { @Test fun `package with no new data comes back ok right away`() = runBlocking { + every { data.close() } just Runs + assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, FLAG_DATA_NOT_CHANGED)) assertTrue(backup.hasState()) + verify { data.close() } + every { plugin.packageFinished(packageInfo) } just Runs assertEquals(TRANSPORT_OK, backup.finishBackup())