Stop writing out old metadata to backend

We'll probably keep metadata around for internal information about backup state
grote · Sep 11, 2024 · b3a88fe · b3a88fe
1 parent 8413f16
commit b3a88fe
Show file tree

Hide file tree

Showing 5 changed files with 6 additions and 48 deletions.
diff --git a/app/src/main/java/com/stevesoltys/seedvault/backend/BackendExt.kt b/app/src/main/java/com/stevesoltys/seedvault/backend/BackendExt.kt
@@ -11,11 +11,6 @@ import org.calyxos.seedvault.core.backends.Backend
 import org.calyxos.seedvault.core.backends.FileHandle
 import org.calyxos.seedvault.core.backends.LegacyAppBackupFile
 import java.io.IOException
-import java.io.OutputStream
-
-suspend fun Backend.getMetadataOutputStream(token: Long): OutputStream {
-    return save(LegacyAppBackupFile.Metadata(token))
-}
 
 suspend fun Backend.getAvailableBackupFileHandles(): List<FileHandle> {
     // v1 get all restore set tokens in root folder that have a metadata file

diff --git a/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt b/app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt
@@ -188,11 +188,10 @@ internal class MetadataManager(
     internal fun onPackageBackupError(
         packageInfo: PackageInfo,
         packageState: PackageState,
-        metadataOutputStream: OutputStream,
         backupType: BackupType? = null,
     ) {
         check(packageState != APK_AND_DATA) { "Backup Error with non-error package state." }
-        modifyMetadata(metadataOutputStream) {
+        modifyCachedMetadata {
             metadata.packageMetadataMap.getOrPut(packageInfo.packageName) {
                 val isSystemApp = packageInfo.isSystemApp()
                 PackageMetadata(
@@ -262,24 +261,7 @@ internal class MetadataManager(
             metadata = oldMetadata
             throw IOException(e)
         }
-    }
-
-    @Throws(IOException::class)
-    private fun modifyMetadata(metadataOutputStream: OutputStream, modFun: () -> Unit) {
-        val oldMetadata = metadata.copy( // copy map, otherwise it will re-use same reference
-            packageMetadataMap = PackageMetadataMap(metadata.packageMetadataMap),
-        )
-        try {
-            modFun.invoke()
-            metadataWriter.write(metadata, metadataOutputStream)
-            writeMetadataToCache()
-        } catch (e: IOException) {
-            Log.w(TAG, "Error writing metadata to storage", e)
-            // revert metadata and do not write it to cache
-            metadata = oldMetadata
-            throw IOException(e)
-        }
-        mLastBackupTime.postValue(metadata.time)
+        mLastBackupTime.postValue(metadata.time) // TODO only do after snapshot was written
     }
 
     /**

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
@@ -23,7 +23,6 @@ import android.util.Log
 import androidx.annotation.WorkerThread
 import com.stevesoltys.seedvault.Clock
 import com.stevesoltys.seedvault.backend.BackendManager
-import com.stevesoltys.seedvault.backend.getMetadataOutputStream
 import com.stevesoltys.seedvault.backend.isOutOfSpace
 import com.stevesoltys.seedvault.metadata.BackupType
 import com.stevesoltys.seedvault.metadata.MetadataManager
@@ -386,13 +385,10 @@ internal class BackupCoordinator(
     }
 
     // TODO is this only nice to have info, or do we need to do more?
-    private suspend fun onPackageBackupError(packageInfo: PackageInfo, type: BackupType) {
+    private fun onPackageBackupError(packageInfo: PackageInfo, type: BackupType) {
         val packageName = packageInfo.packageName
         try {
-            val token = settingsManager.getToken() ?: error("no token")
-            backend.getMetadataOutputStream(token).use {
-                metadataManager.onPackageBackupError(packageInfo, state.cancelReason, it, type)
-            }
+            metadataManager.onPackageBackupError(packageInfo, state.cancelReason, type)
         } catch (e: IOException) {
             Log.e(TAG, "Error while writing metadata for $packageName", e)
         }

diff --git a/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt b/app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt
@@ -472,7 +472,7 @@ class MetadataManagerTest {
         expectReadFromCache()
         expectModifyMetadata(updatedMetadata)
 
-        manager.onPackageBackupError(packageInfo, NO_DATA, storageOutputStream, BackupType.KV)
+        manager.onPackageBackupError(packageInfo, NO_DATA, BackupType.KV)
     }
 
     @Test
@@ -485,7 +485,7 @@ class MetadataManagerTest {
         expectReadFromCache()
         expectModifyMetadata(updatedMetadata)
 
-        manager.onPackageBackupError(packageInfo, WAS_STOPPED, storageOutputStream)
+        manager.onPackageBackupError(packageInfo, WAS_STOPPED)
     }
 
     @Test

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
@@ -32,12 +32,10 @@ import io.mockk.mockk
 import io.mockk.verify
 import kotlinx.coroutines.runBlocking
 import org.calyxos.seedvault.core.backends.Backend
-import org.calyxos.seedvault.core.backends.LegacyAppBackupFile
 import org.calyxos.seedvault.core.backends.saf.SafProperties
 import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Test
 import java.io.IOException
-import java.io.OutputStream
 import kotlin.random.Random
 
 internal class BackupCoordinatorTest : BackupTest() {
@@ -64,7 +62,6 @@ internal class BackupCoordinatorTest : BackupTest() {
     )
 
     private val backend = mockk<Backend>()
-    private val metadataOutputStream = mockk<OutputStream>()
     private val fileDescriptor: ParcelFileDescriptor = mockk()
     private val packageMetadata: PackageMetadata = mockk()
     private val safProperties = SafProperties(
@@ -211,12 +208,9 @@ internal class BackupCoordinatorTest : BackupTest() {
             metadataManager.onPackageBackupError(
                 packageInfo,
                 UNKNOWN_ERROR,
-                metadataOutputStream,
                 BackupType.KV,
             )
         } just Runs
-        coEvery { backend.save(LegacyAppBackupFile.Metadata(token)) } returns metadataOutputStream
-        every { metadataOutputStream.close() } just Runs
 
         assertEquals(TRANSPORT_PACKAGE_REJECTED, backup.finishBackup())
     }
@@ -270,14 +264,12 @@ internal class BackupCoordinatorTest : BackupTest() {
             metadataManager.onPackageBackupError(
                 packageInfo,
                 QUOTA_EXCEEDED,
-                metadataOutputStream,
                 BackupType.FULL
             )
         } just Runs
         coEvery { full.cancelFullBackup() } just Runs
         every { backendManager.backendProperties } returns safProperties
         every { settingsManager.useMeteredNetwork } returns false
-        every { metadataOutputStream.close() } just Runs
 
         assertEquals(
             TRANSPORT_OK,
@@ -298,11 +290,9 @@ internal class BackupCoordinatorTest : BackupTest() {
             metadataManager.onPackageBackupError(
                 packageInfo,
                 QUOTA_EXCEEDED,
-                metadataOutputStream,
                 BackupType.FULL
             )
         }
-        verify { metadataOutputStream.close() }
     }
 
     @Test
@@ -318,14 +308,12 @@ internal class BackupCoordinatorTest : BackupTest() {
             metadataManager.onPackageBackupError(
                 packageInfo,
                 NO_DATA,
-                metadataOutputStream,
                 BackupType.FULL
             )
         } just Runs
         coEvery { full.cancelFullBackup() } just Runs
         every { backendManager.backendProperties } returns safProperties
         every { settingsManager.useMeteredNetwork } returns false
-        every { metadataOutputStream.close() } just Runs
 
         assertEquals(
             TRANSPORT_OK,
@@ -343,11 +331,9 @@ internal class BackupCoordinatorTest : BackupTest() {
             metadataManager.onPackageBackupError(
                 packageInfo,
                 NO_DATA,
-                metadataOutputStream,
                 BackupType.FULL
             )
         }
-        verify { metadataOutputStream.close() }
     }
 
     @Test
@@ -357,7 +343,6 @@ internal class BackupCoordinatorTest : BackupTest() {
     private fun expectApkBackupAndMetadataWrite() {
         coEvery { apkBackup.backupApkIfNecessary(packageInfo) } just Runs
         every { settingsManager.getToken() } returns token
-        coEvery { backend.save(LegacyAppBackupFile.Metadata(token)) } returns metadataOutputStream
         every { metadataManager.onApkBackedUp(any(), packageMetadata) } just Runs
     }