From 87a800438b2f90f8e665c3ff1f5cbf67dda1e25a Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Tue, 3 Jan 2023 16:29:17 -0300 Subject: [PATCH] Always start a new RestoreSet when initializing the device This avoids deleting the current backup when the user disables backups (or the system decides to do a random re-init). --- .../transport/backup/BackupCoordinator.kt | 23 ++++++++-------- .../ui/recoverycode/RecoveryCodeViewModel.kt | 3 --- .../ui/storage/BackupStorageViewModel.kt | 3 --- .../transport/backup/BackupCoordinatorTest.kt | 27 +++++-------------- 4 files changed, 19 insertions(+), 37 deletions(-) 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 5d791fc4..de4287ba 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 @@ -88,13 +88,16 @@ internal class BackupCoordinator( * Starts a new [RestoreSet] with a new token (the current unix epoch in milliseconds). * Call this at least once before calling [initializeDevice] * which must be called after this method to properly initialize the backup transport. + * + * @return the token of the new [RestoreSet]. */ @Throws(IOException::class) - suspend fun startNewRestoreSet() { + private suspend fun startNewRestoreSet(): Long { val token = clock.time() Log.i(TAG, "Starting new RestoreSet with token $token...") settingsManager.setNewToken(token) plugin.startNewRestoreSet(token) + return token } /** @@ -116,16 +119,14 @@ internal class BackupCoordinator( * [TRANSPORT_ERROR] (to retry following network error or other failure). */ suspend fun initializeDevice(): Int = try { - val token = settingsManager.getToken() - if (token == null) { - Log.i(TAG, "No RestoreSet started, initialization is no-op.") - } else { - Log.i(TAG, "Initialize Device!") - plugin.initializeDevice() - Log.d(TAG, "Resetting backup metadata for token $token...") - plugin.getMetadataOutputStream().use { - metadataManager.onDeviceInitialization(token, it) - } + // 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() + Log.i(TAG, "Initialize Device!") + plugin.initializeDevice() + Log.d(TAG, "Resetting backup metadata for token $token...") + plugin.getMetadataOutputStream(token).use { + metadataManager.onDeviceInitialization(token, it) } // [finishBackup] will only be called when we return [TRANSPORT_OK] here // so we remember that we initialized successfully 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 index 02c69915..5dbc82ef 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/ui/recoverycode/RecoveryCodeViewModel.kt @@ -107,9 +107,6 @@ internal class RecoveryCodeViewModel( storageBackup.deleteAllSnapshots() storageBackup.clearCache() try { - // will also generate a new backup token for the new restore set - backupCoordinator.startNewRestoreSet() - // initialize the new location if (backupManager.isBackupEnabled) backupManager.initializeTransportsForUser( UserHandle.myUserId(), 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 index 27015263..4595468b 100644 --- a/app/src/main/java/com/stevesoltys/seedvault/ui/storage/BackupStorageViewModel.kt +++ b/app/src/main/java/com/stevesoltys/seedvault/ui/storage/BackupStorageViewModel.kt @@ -38,9 +38,6 @@ internal class BackupStorageViewModel( storageBackup.deleteAllSnapshots() storageBackup.clearCache() try { - // will also generate a new backup token for the new restore set - backupCoordinator.startNewRestoreSet() - // initialize the new location (if backups are enabled) if (backupManager.isBackupEnabled) backupManager.initializeTransportsForUser( UserHandle.myUserId(), 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 d5facae7..5a7ece97 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 @@ -72,18 +72,9 @@ internal class BackupCoordinatorTest : BackupTest() { requiresNetwork = false ) - @Test - fun `starting a new restore set works as expected`() = runBlocking { - every { clock.time() } returns token - every { settingsManager.setNewToken(token) } just Runs - coEvery { plugin.startNewRestoreSet(token) } just Runs - - backup.startNewRestoreSet() - } - @Test fun `device initialization succeeds and delegates to plugin`() = runBlocking { - every { settingsManager.getToken() } returns token + expectStartNewRestoreSet() coEvery { plugin.initializeDevice() } just Runs coEvery { plugin.getOutputStream(token, FILE_BACKUP_METADATA) } returns metadataOutputStream every { metadataManager.onDeviceInitialization(token, metadataOutputStream) } just Runs @@ -97,21 +88,17 @@ internal class BackupCoordinatorTest : BackupTest() { verify { metadataOutputStream.close() } } - @Test - fun `device initialization does no-op when no token available`() = runBlocking { - every { settingsManager.getToken() } returns null - every { kv.hasState() } returns false - every { full.hasState() } returns false - - assertEquals(TRANSPORT_OK, backup.initializeDevice()) - assertEquals(TRANSPORT_OK, backup.finishBackup()) + private suspend fun expectStartNewRestoreSet() { + every { clock.time() } returns token + every { settingsManager.setNewToken(token) } just Runs + coEvery { plugin.startNewRestoreSet(token) } just Runs } @Test fun `error notification when device initialization fails`() = runBlocking { val maybeTrue = Random.nextBoolean() - every { settingsManager.getToken() } returns token + expectStartNewRestoreSet() coEvery { plugin.initializeDevice() } throws IOException() every { metadataManager.requiresInit } returns maybeTrue every { settingsManager.canDoBackupNow() } returns !maybeTrue @@ -130,7 +117,7 @@ internal class BackupCoordinatorTest : BackupTest() { @Test fun `no error notification when device initialization fails when no backup possible`() = runBlocking { - every { settingsManager.getToken() } returns token + expectStartNewRestoreSet() coEvery { plugin.initializeDevice() } throws IOException() every { metadataManager.requiresInit } returns false every { settingsManager.canDoBackupNow() } returns false