Pretend to make successful @pm@ backup when no backup is possible

This is the same behavior as Google backup when it has no internet connection and after extensive research the only option we can keep the system from considering the backup state to be compromised.

K/V backups are run at least every day, no matter what backup interval we set in settings and when they run, we don't get asked before, if now is a good time for backups. So we need to fake an OK for @pm@ backup and can error out afterwards without compromising state.

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 {
+    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)
+    }
 }
 
 /**

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 = withContext(Dispatchers.IO) {
-            val enabled = storage != null && storageAvailable(storage)
+                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)
-    }
-
 }

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)
     }

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 val state = CoordinatorState(
-    private var calledClearBackupData = false
+        calledInitialize = false,
-    private var cancelReason: PackageState = UNKNOWN_ERROR
+        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.
+        // We need to reject them manually when we can not do a backup now.
-        if (packageName == MAGIC_PACKAGE_MANAGER && getBackupBackoff() != 0L) {
+        // What else we tried can be found in: https://github.com/stevesoltys/seedvault/issues/102
-            return TRANSPORT_PACKAGE_REJECTED
+        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
+        if (result == TRANSPORT_PACKAGE_REJECTED) state.cancelReason = NO_DATA
-        else if (result == TRANSPORT_QUOTA_EXCEEDED) cancelReason = QUOTA_EXCEEDED
+        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.
+     * Finish sending application data to the backup destination. This must be called
-     * This must be called after [performIncrementalBackup], [performFullBackup], or [clearBackupData]
+     * 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 -> {
+        state.expectFinish -> {
-            calledInitialize = false
+            state.onFinish()
-            calledClearBackupData = false
             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)

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 {

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<OutputStream>()
     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<Storage>()
-
             every { settingsManager.getToken() } returns token
             coEvery { plugin.initializeDevice() } throws IOException()
-            every { settingsManager.getStorage() } returns storage
+            every { settingsManager.canDoBackupNow() } returns false
-            every { storage.isUnavailableUsb(context) } returns true
 
             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

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())