Stop writing out old metadata to backend

We'll probably keep metadata around for internal information about backup state

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

app/src/main/java/com/stevesoltys/seedvault/metadata/MetadataManager.kt

@@ -100,7 +100,6 @@ internal class MetadataManager(
      * Call this after a package's APK has been backed up successfully.
      *
      * It updates the packages' metadata to the internal cache.
-     * You still need to call [uploadMetadata] to persist all local modifications.
      */
     @Synchronized
     @Throws(IOException::class)
@@ -188,11 +187,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(
@@ -212,7 +210,6 @@ internal class MetadataManager(
      * Call this for all packages we can not back up for some reason.
      *
      * It updates the packages' local metadata.
-     * You still need to call [uploadMetadata] to persist all local modifications.
      */
     @Synchronized
     @Throws(IOException::class)
@@ -239,15 +236,6 @@ internal class MetadataManager(
         }
     }
 
-    /**
-     * Uploads metadata to given [metadataOutputStream] after performing local modifications.
-     */
-    @Synchronized
-    @Throws(IOException::class)
-    fun uploadMetadata(metadataOutputStream: OutputStream) {
-        metadataWriter.write(metadata, metadataOutputStream)
-    }
-
     @Throws(IOException::class)
     private fun modifyCachedMetadata(modFun: () -> Unit) {
         val oldMetadata = metadata.copy( // copy map, otherwise it will re-use same reference
@@ -262,24 +250,7 @@ internal class MetadataManager(
             metadata = oldMetadata
             throw IOException(e)
         }
-    }
+        mLastBackupTime.postValue(metadata.time) // TODO only do after snapshot was written
-
-    @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)
     }
 
     /**

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")
+            metadataManager.onPackageBackupError(packageInfo, state.cancelReason, type)
-            backend.getMetadataOutputStream(token).use {
-                metadataManager.onPackageBackupError(packageInfo, state.cancelReason, it, type)
-            }
         } catch (e: IOException) {
             Log.e(TAG, "Error while writing metadata for $packageName", e)
         }

app/src/test/java/com/stevesoltys/seedvault/metadata/MetadataManagerTest.kt

@@ -473,7 +473,7 @@ class MetadataManagerTest {
         expectReadFromCache()
         expectModifyMetadata(updatedMetadata)
 
-        manager.onPackageBackupError(packageInfo, NO_DATA, storageOutputStream, BackupType.KV)
+        manager.onPackageBackupError(packageInfo, NO_DATA, BackupType.KV)
     }
 
     @Test
@@ -486,22 +486,7 @@ class MetadataManagerTest {
         expectReadFromCache()
         expectModifyMetadata(updatedMetadata)
 
-        manager.onPackageBackupError(packageInfo, WAS_STOPPED, storageOutputStream)
+        manager.onPackageBackupError(packageInfo, WAS_STOPPED)
-    }
-
-    @Test
-    fun `test uploadMetadata() uploads`() {
-        expectReadFromCache()
-        every { metadataWriter.write(initialMetadata, storageOutputStream) } just Runs
-
-        manager.uploadMetadata(storageOutputStream)
-    }
-
-    @Test
-    fun `test getBackupToken() on first run`() {
-        every { context.openFileInput(METADATA_CACHE_FILE) } throws FileNotFoundException()
-
-        assertEquals(0L, manager.getBackupToken())
     }
 
     @Test

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
     }