WIP: Request @pm@ backup after initialization

to avoid a 2nd restore set being used. This also changes the initialization behavior to only create the restore set folder and upload the metadata only when we actually need to. This way, double inits are not creating new restore sets on the backup destination.
grote · Feb 26, 2024 · 9570c95 · 9570c95
1 parent cef3936
commit 9570c95
Show file tree

Hide file tree

Showing 11 changed files with 115 additions and 79 deletions.
diff --git a/app/src/androidTest/java/com/stevesoltys/seedvault/KoinInstrumentationTestApp.kt b/app/src/androidTest/java/com/stevesoltys/seedvault/KoinInstrumentationTestApp.kt
@@ -47,7 +47,7 @@ class KoinInstrumentationTestApp : App() {
 
             viewModel {
                 currentBackupStorageViewModel =
-                    spyk(BackupStorageViewModel(context, get(), get(), get()))
+                    spyk(BackupStorageViewModel(context, get(), get(), get(), get()))
                 currentBackupStorageViewModel!!
             }
 

diff --git a/app/src/main/java/com/stevesoltys/seedvault/App.kt b/app/src/main/java/com/stevesoltys/seedvault/App.kt
@@ -59,7 +59,7 @@ open class App : Application() {
 
         viewModel { SettingsViewModel(this@App, get(), get(), get(), get(), get(), get()) }
         viewModel { RecoveryCodeViewModel(this@App, get(), get(), get(), get(), get(), get()) }
-        viewModel { BackupStorageViewModel(this@App, get(), get(), get()) }
+        viewModel { BackupStorageViewModel(this@App, get(), get(), get(), get()) }
         viewModel { RestoreStorageViewModel(this@App, get(), get()) }
         viewModel { RestoreViewModel(this@App, get(), get(), get(), get(), get(), get()) }
         viewModel { FileSelectionViewModel(this@App, get()) }

diff --git a/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt b/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt
@@ -59,14 +59,15 @@ internal class MetadataManager(
     /**
      * Call this when initializing a new device.
      *
-     * Existing [BackupMetadata] will be cleared, use the given new token,
-     * and written encrypted to the given [OutputStream] as well as the internal cache.
+     * Existing [BackupMetadata] will be cleared
+     * and new metadata with the given [token] will be written to the internal cache
+     * with a fresh salt.
      */
     @Synchronized
     @Throws(IOException::class)
-    fun onDeviceInitialization(token: Long, metadataOutputStream: OutputStream) {
+    fun onDeviceInitialization(token: Long) {
         val salt = crypto.getRandomBytes(METADATA_SALT_SIZE).encodeBase64()
-        modifyMetadata(metadataOutputStream) {
+        modifyCachedMetadata {
             metadata = BackupMetadata(token = token, salt = salt)
         }
     }

diff --git a/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderStoragePlugin.kt b/app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderStoragePlugin.kt
@@ -35,22 +35,13 @@ internal class DocumentsProviderStoragePlugin(
     override suspend fun startNewRestoreSet(token: Long) {
         // reset current storage
         storage.reset(token)
-
-        // get or create root backup dir
-        storage.rootBackupDir ?: throw IOException()
     }
 
     @Throws(IOException::class)
     override suspend fun initializeDevice() {
-        // wipe existing data
-        storage.getSetDir()?.deleteContents(context)
-
         // reset storage without new token, so folders get recreated
         // otherwise stale DocumentFiles will hang around
         storage.reset(null)
-
-        // create backup folders
-        storage.currentSetDir ?: throw IOException()
     }
 
     @Throws(IOException::class)

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
@@ -86,12 +86,13 @@ internal class BackupCoordinator(
      * @return the token of the new [RestoreSet].
      */
     @Throws(IOException::class)
-    private suspend fun startNewRestoreSet(): Long {
+    private suspend fun startNewRestoreSet() {
         val token = clock.time()
         Log.i(TAG, "Starting new RestoreSet with token $token...")
         settingsManager.setNewToken(token)
         plugin.startNewRestoreSet(token)
-        return token
+        Log.d(TAG, "Resetting backup metadata...")
+        metadataManager.onDeviceInitialization(token)
     }
 
     /**
@@ -115,18 +116,15 @@ internal class BackupCoordinator(
     suspend fun initializeDevice(): Int = try {
         // we don't respect the intended system behavior here by always starting a new [RestoreSet]
         // instead of simply deleting the current one
-        val token = startNewRestoreSet()
+        startNewRestoreSet()
         Log.i(TAG, "Initialize Device!")
         plugin.initializeDevice()
-        Log.d(TAG, "Resetting backup metadata for token $token...")
-        plugin.getMetadataOutputStream(token).use {
-            metadataManager.onDeviceInitialization(token, it)
-        }
+        startNewRestoreSet()
         // [finishBackup] will only be called when we return [TRANSPORT_OK] here
         // so we remember that we initialized successfully
         state.calledInitialize = true
         TRANSPORT_OK
-    } catch (e: IOException) {
+    } catch (e: Exception) {
         Log.e(TAG, "Error initializing device", e)
         // Show error notification if we needed init or were ready for backups
         if (metadataManager.requiresInit || settingsManager.canDoBackupNow()) nm.onBackupError()
@@ -222,6 +220,7 @@ internal class BackupCoordinator(
         state.cancelReason = UNKNOWN_ERROR
         if (metadataManager.requiresInit) {
             Log.w(TAG, "Metadata requires re-init!")
+            // TODO test if this still works
             // start a new restore set to upgrade from legacy format
             // by starting a clean backup with all files using the new version
             try {

diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupInitializer.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupInitializer.kt
@@ -0,0 +1,73 @@
+/*
+ * SPDX-FileCopyrightText: 2024 The Calyx Institute
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package com.stevesoltys.seedvault.transport.backup
+
+import android.app.backup.BackupProgress
+import android.app.backup.IBackupManager
+import android.app.backup.IBackupObserver
+import android.os.UserHandle
+import android.util.Log
+import androidx.annotation.WorkerThread
+import com.stevesoltys.seedvault.BackupMonitor
+import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER
+import com.stevesoltys.seedvault.transport.TRANSPORT_ID
+
+class BackupInitializer(
+    private val backupManager: IBackupManager,
+) {
+
+    companion object {
+        private val TAG = BackupInitializer::class.simpleName
+    }
+
+    fun initialize(onError: () -> Unit, onSuccess: () -> Unit) {
+        val observer = BackupObserver("Initialization", onError) {
+            // After successful initialization, we request a @pm@ backup right away,
+            // because if this finds empty state, it asks us to do another initialization.
+            // And then we end up with yet another restore set token.
+            // Since we want the final token as soon as possible, we need to get it here.
+            Log.d(TAG, "Requesting initial $MAGIC_PACKAGE_MANAGER backup...")
+            backupManager.requestBackup(
+                arrayOf(MAGIC_PACKAGE_MANAGER),
+                BackupObserver("Initial backup of @pm@", onError, onSuccess),
+                BackupMonitor(),
+                0,
+            )
+        }
+        backupManager.initializeTransportsForUser(
+            UserHandle.myUserId(),
+            arrayOf(TRANSPORT_ID),
+            observer,
+        )
+    }
+
+    @WorkerThread
+    private inner class BackupObserver(
+        private val operation: String,
+        private val onError: () -> Unit,
+        private val onSuccess: () -> Unit,
+    ) : IBackupObserver.Stub() {
+        override fun onUpdate(currentBackupPackage: String, backupProgress: BackupProgress) {
+            // noop
+        }
+
+        override fun onResult(target: String, status: Int) {
+            // noop
+        }
+
+        override fun backupFinished(status: Int) {
+            if (Log.isLoggable(TAG, Log.INFO)) {
+                Log.i(TAG, "$operation finished. Status: $status")
+            }
+            if (status == 0) {
+                onSuccess()
+            } else {
+                onError()
+            }
+        }
+    }
+
+}
diff --git a/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupModule.kt b/app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupModule.kt
@@ -4,6 +4,7 @@ import org.koin.android.ext.koin.androidContext
 import org.koin.dsl.module
 
 val backupModule = module {
+    single { BackupInitializer(get()) }
     single { InputFactory() }
     single {
         PackageService(

diff --git a/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt b/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt
@@ -1,7 +1,6 @@
 package com.stevesoltys.seedvault.ui.recoverycode
 
 import android.app.backup.IBackupManager
-import android.os.UserHandle
 import android.util.Log
 import androidx.lifecycle.AndroidViewModel
 import cash.z.ecc.android.bip39.Mnemonics
@@ -12,8 +11,7 @@ import cash.z.ecc.android.bip39.toSeed
 import com.stevesoltys.seedvault.App
 import com.stevesoltys.seedvault.crypto.Crypto
 import com.stevesoltys.seedvault.crypto.KeyManager
-import com.stevesoltys.seedvault.transport.TRANSPORT_ID
-import com.stevesoltys.seedvault.transport.backup.BackupCoordinator
+import com.stevesoltys.seedvault.transport.backup.BackupInitializer
 import com.stevesoltys.seedvault.ui.LiveEvent
 import com.stevesoltys.seedvault.ui.MutableLiveEvent
 import com.stevesoltys.seedvault.ui.notification.BackupNotificationManager
@@ -32,7 +30,7 @@ internal class RecoveryCodeViewModel(
     private val crypto: Crypto,
     private val keyManager: KeyManager,
     private val backupManager: IBackupManager,
-    private val backupCoordinator: BackupCoordinator,
+    private val backupInitializer: BackupInitializer,
     private val notificationManager: BackupNotificationManager,
     private val storageBackup: StorageBackup,
 ) : AndroidViewModel(app) {
@@ -109,11 +107,9 @@ internal class RecoveryCodeViewModel(
             storageBackup.clearCache()
             try {
                 // initialize the new location
-                if (backupManager.isBackupEnabled) backupManager.initializeTransportsForUser(
-                    UserHandle.myUserId(),
-                    arrayOf(TRANSPORT_ID),
-                    null
-                )
+                if (backupManager.isBackupEnabled) backupInitializer.initialize({ }) {
+                    // no-op
+                }
             } catch (e: IOException) {
                 Log.e(TAG, "Error starting new RestoreSet", e)
             }

diff --git a/app/src/main/java/com/stevesoltys/seedvault/ui/storage/BackupStorageViewModel.kt b/app/src/main/java/com/stevesoltys/seedvault/ui/storage/BackupStorageViewModel.kt
@@ -1,20 +1,16 @@
 package com.stevesoltys.seedvault.ui.storage
 
 import android.app.Application
-import android.app.backup.BackupProgress
 import android.app.backup.IBackupManager
-import android.app.backup.IBackupObserver
 import android.app.job.JobInfo
 import android.net.Uri
-import android.os.UserHandle
 import android.util.Log
-import androidx.annotation.WorkerThread
 import androidx.lifecycle.viewModelScope
 import androidx.work.ExistingPeriodicWorkPolicy.CANCEL_AND_REENQUEUE
 import com.stevesoltys.seedvault.R
 import com.stevesoltys.seedvault.settings.SettingsManager
 import com.stevesoltys.seedvault.storage.StorageBackupJobService
-import com.stevesoltys.seedvault.transport.TRANSPORT_ID
+import com.stevesoltys.seedvault.transport.backup.BackupInitializer
 import com.stevesoltys.seedvault.worker.AppBackupWorker
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
@@ -28,6 +24,7 @@ private val TAG = BackupStorageViewModel::class.java.simpleName
 internal class BackupStorageViewModel(
     private val app: Application,
     private val backupManager: IBackupManager,
+    private val backupInitializer: BackupInitializer,
     private val storageBackup: StorageBackup,
     settingsManager: SettingsManager,
 ) : StorageViewModel(app, settingsManager) {
@@ -52,13 +49,23 @@ internal class BackupStorageViewModel(
             storageBackup.clearCache()
             try {
                 // initialize the new location (if backups are enabled)
-                if (backupManager.isBackupEnabled) backupManager.initializeTransportsForUser(
-                    UserHandle.myUserId(),
-                    arrayOf(TRANSPORT_ID),
-                    // if storage is on USB and this is not SetupWizard, do a backup right away
-                    InitializationObserver(isUsb && !isSetupWizard)
-                ) else {
-                    InitializationObserver(false).backupFinished(0)
+                if (backupManager.isBackupEnabled) {
+                    val onError = {
+                        Log.e(TAG, "Error starting new RestoreSet")
+                        onInitializationError()
+                    }
+                    backupInitializer.initialize(onError) {
+                        val requestBackup = isUsb && !isSetupWizard
+                        if (requestBackup) {
+                            Log.i(TAG, "Requesting a backup now, because we use USB storage")
+                            AppBackupWorker.scheduleNow(app, reschedule = false)
+                        }
+                        // notify the UI that the location has been set
+                        mLocationChecked.postEvent(LocationResult())
+                    }
+                } else {
+                    // notify the UI that the location has been set
+                    mLocationChecked.postEvent(LocationResult())
                 }
             } catch (e: IOException) {
                 Log.e(TAG, "Error starting new RestoreSet", e)
@@ -90,35 +97,6 @@ internal class BackupStorageViewModel(
         BackupJobService.cancelJob(app)
     }
 
-    @WorkerThread
-    private inner class InitializationObserver(val requestBackup: Boolean) :
-        IBackupObserver.Stub() {
-        override fun onUpdate(currentBackupPackage: String, backupProgress: BackupProgress) {
-            // noop
-        }
-
-        override fun onResult(target: String, status: Int) {
-            // noop
-        }
-
-        override fun backupFinished(status: Int) {
-            if (Log.isLoggable(TAG, Log.INFO)) {
-                Log.i(TAG, "Initialization finished. Status: $status")
-            }
-            if (status == 0) {
-                // notify the UI that the location has been set
-                mLocationChecked.postEvent(LocationResult())
-                if (requestBackup) {
-                    val isUsb = settingsManager.getStorage()?.isUsb ?: false
-                    AppBackupWorker.scheduleNow(app, reschedule = !isUsb)
-                }
-            } else {
-                // notify the UI that the location was invalid
-                onInitializationError()
-            }
-        }
-    }
-
     private fun onInitializationError() {
         val errorMsg = app.getString(R.string.storage_check_fragment_backup_error)
         mLocationChecked.postEvent(LocationResult(errorMsg))

diff --git a/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt b/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt
@@ -100,7 +100,7 @@ class MetadataManagerTest {
         expectReadFromCache()
         expectModifyMetadata(initialMetadata)
 
-        manager.onDeviceInitialization(token, storageOutputStream)
+        manager.onDeviceInitialization(token)
 
         assertEquals(token, manager.getBackupToken())
         assertEquals(0L, manager.getLastBackupTime())

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
@@ -69,22 +69,18 @@ internal class BackupCoordinatorTest : BackupTest() {
     fun `device initialization succeeds and delegates to plugin`() = runBlocking {
         expectStartNewRestoreSet()
         coEvery { plugin.initializeDevice() } just Runs
-        coEvery { plugin.getOutputStream(token, FILE_BACKUP_METADATA) } returns metadataOutputStream
-        every { metadataManager.onDeviceInitialization(token, metadataOutputStream) } just Runs
         every { kv.hasState() } returns false
         every { full.hasState() } returns false
-        every { metadataOutputStream.close() } just Runs
 
         assertEquals(TRANSPORT_OK, backup.initializeDevice())
         assertEquals(TRANSPORT_OK, backup.finishBackup())
-
-        verify { metadataOutputStream.close() }
     }
 
     private suspend fun expectStartNewRestoreSet() {
         every { clock.time() } returns token
         every { settingsManager.setNewToken(token) } just Runs
         coEvery { plugin.startNewRestoreSet(token) } just Runs
+        every { metadataManager.onDeviceInitialization(token) } just Runs
     }
 
     @Test
@@ -136,6 +132,7 @@ internal class BackupCoordinatorTest : BackupTest() {
         every { clock.time() } returns token + 1
         every { settingsManager.setNewToken(token + 1) } just Runs
         coEvery { plugin.startNewRestoreSet(token + 1) } just Runs
+        every { metadataManager.onDeviceInitialization(token + 1) } just Runs
 
         every { data.close() } just Runs