Ensure streams get closed eventually

app/src/main/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinator.kt

@@ -77,7 +77,9 @@ internal class BackupCoordinator(
             val token = clock.time()
             if (plugin.initializeDevice(token)) {
                 Log.d(TAG, "Resetting backup metadata...")
-                metadataManager.onDeviceInitialization(token, plugin.getMetadataOutputStream())
+                plugin.getMetadataOutputStream().use {
+                    metadataManager.onDeviceInitialization(token, it)
+                }
             } else {
                 Log.d(TAG, "Storage was already initialized, doing no-op")
             }
@@ -302,8 +304,9 @@ internal class BackupCoordinator(
             apkBackup.backupApkIfNecessary(packageInfo, packageState) {
                 plugin.getApkOutputStream(packageInfo)
             }?.let { packageMetadata ->
-                val outputStream = plugin.getMetadataOutputStream()
+                plugin.getMetadataOutputStream().use {
-                metadataManager.onApkBackedUp(packageInfo, packageMetadata, outputStream)
+                    metadataManager.onApkBackedUp(packageInfo, packageMetadata, it)
+                }
             }
         } catch (e: IOException) {
             Log.e(TAG, "Error while writing APK or metadata for $packageName", e)
@@ -313,8 +316,9 @@ internal class BackupCoordinator(
     private suspend fun onPackageBackedUp(packageInfo: PackageInfo) {
         val packageName = packageInfo.packageName
         try {
-            val outputStream = plugin.getMetadataOutputStream()
+            plugin.getMetadataOutputStream().use {
-            metadataManager.onPackageBackedUp(packageInfo, outputStream)
+                metadataManager.onPackageBackedUp(packageInfo, it)
+            }
         } catch (e: IOException) {
             Log.e(TAG, "Error while writing metadata for $packageName", e)
         }
@@ -325,8 +329,9 @@ internal class BackupCoordinator(
         if (cancelReason == NO_DATA && packageInfo.isSystemApp()) return
         val packageName = packageInfo.packageName
         try {
-            val outputStream = plugin.getMetadataOutputStream()
+            plugin.getMetadataOutputStream().use {
-            metadataManager.onPackageBackupError(packageInfo, cancelReason, outputStream)
+                metadataManager.onPackageBackupError(packageInfo, 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

@@ -114,15 +114,26 @@ internal class KVBackup(
                     plugin.deleteRecord(packageInfo, op.base64Key)
                 } else {
                     val outputStream = plugin.getOutputStreamForRecord(packageInfo, op.base64Key)
-                    val header = VersionHeader(packageName = packageInfo.packageName, key = op.key)
+                    try {
+                        val header = VersionHeader(
+                            packageName = packageInfo.packageName,
+                            key = op.key
+                        )
                         headerWriter.writeVersion(outputStream, header)
                         crypto.encryptHeader(outputStream, header)
                         crypto.encryptMultipleSegments(outputStream, op.value)
                         outputStream.flush()
+                    } finally {
                         closeQuietly(outputStream)
                     }
+                }
             } catch (e: IOException) {
                 Log.e(TAG, "Unable to update base64Key file for base64Key ${op.base64Key}", e)
+                // Returning something more forgiving such as TRANSPORT_PACKAGE_REJECTED
+                // will still make the entire backup fail.
+                // TODO However, TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED might buy us a retry,
+                //  we would just need to be careful not to create an infinite loop
+                //  for permanent errors.
                 return backupError(TRANSPORT_ERROR)
             }
         }

app/src/main/java/com/stevesoltys/seedvault/transport/restore/KVRestore.kt

@@ -143,10 +143,8 @@ internal class KVRestore(
         state: KVRestoreState,
         dKey: DecodedKey,
         out: BackupDataOutput
-    ) {
+    ) = plugin.getInputStreamForRecord(state.token, state.packageInfo, dKey.base64Key)
-        val inputStream =
+        .use { inputStream ->
-            plugin.getInputStreamForRecord(state.token, state.packageInfo, dKey.base64Key)
-        try {
             val version = headerReader.readVersion(inputStream)
             crypto.decryptHeader(inputStream, version, state.packageInfo.packageName, dKey.key)
             val value = crypto.decryptMultipleSegments(inputStream)
@@ -155,9 +153,7 @@ internal class KVRestore(
 
             out.writeEntityHeader(dKey.key, size)
             out.writeEntityData(value, size)
-        } finally {
+            Unit
-            closeQuietly(inputStream)
-        }
         }
 
     private class DecodedKey(internal val base64Key: String) : Comparable<DecodedKey> {

app/src/test/java/com/stevesoltys/seedvault/transport/backup/BackupCoordinatorTest.kt

@@ -70,9 +70,12 @@ internal class BackupCoordinatorTest : BackupTest() {
         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() }
     }
 
     @Test
@@ -137,7 +140,10 @@ internal class BackupCoordinatorTest : BackupTest() {
         } else {
             every { kv.getQuota() } returns quota
         }
+        every { metadataOutputStream.close() } just Runs
         assertEquals(quota, backup.getBackupQuota(packageInfo.packageName, isFullBackup))
+
+        verify { metadataOutputStream.close() }
     }
 
     @Test
@@ -192,8 +198,11 @@ internal class BackupCoordinatorTest : BackupTest() {
         coEvery { plugin.getMetadataOutputStream() } returns metadataOutputStream
         every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs
         every { kv.finishBackup() } returns result
+        every { metadataOutputStream.close() } just Runs
 
         assertEquals(result, backup.finishBackup())
+
+        verify { metadataOutputStream.close() }
     }
 
     @Test
@@ -206,8 +215,11 @@ internal class BackupCoordinatorTest : BackupTest() {
         coEvery { plugin.getMetadataOutputStream() } returns metadataOutputStream
         every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs
         every { full.finishBackup() } returns result
+        every { metadataOutputStream.close() } just Runs
 
         assertEquals(result, backup.finishBackup())
+
+        verify { metadataOutputStream.close() }
     }
 
     @Test
@@ -234,6 +246,7 @@ internal class BackupCoordinatorTest : BackupTest() {
         } just Runs
         coEvery { full.cancelFullBackup() } just Runs
         every { settingsManager.getStorage() } returns storage
+        every { metadataOutputStream.close() } just Runs
 
         assertEquals(
             TRANSPORT_OK,
@@ -253,6 +266,7 @@ internal class BackupCoordinatorTest : BackupTest() {
         verify(exactly = 1) {
             metadataManager.onPackageBackupError(packageInfo, QUOTA_EXCEEDED, metadataOutputStream)
         }
+        verify { metadataOutputStream.close() }
     }
 
     @Test
@@ -271,6 +285,7 @@ internal class BackupCoordinatorTest : BackupTest() {
         } just Runs
         coEvery { full.cancelFullBackup() } just Runs
         every { settingsManager.getStorage() } returns storage
+        every { metadataOutputStream.close() } just Runs
 
         assertEquals(
             TRANSPORT_OK,
@@ -287,6 +302,7 @@ internal class BackupCoordinatorTest : BackupTest() {
         verify(exactly = 1) {
             metadataManager.onPackageBackupError(packageInfo, NO_DATA, metadataOutputStream)
         }
+        verify { metadataOutputStream.close() }
     }
 
     @Test
@@ -326,6 +342,7 @@ internal class BackupCoordinatorTest : BackupTest() {
         } just Runs
         // do actual @pm@ backup
         coEvery { kv.performBackup(packageInfo, fileDescriptor, 0) } returns TRANSPORT_OK
+        every { metadataOutputStream.close() } just Runs
 
         assertEquals(
             TRANSPORT_OK,
@@ -335,6 +352,7 @@ internal class BackupCoordinatorTest : BackupTest() {
         coVerify {
             apkBackup.backupApkIfNecessary(notAllowedPackages[0], NOT_ALLOWED, any())
             apkBackup.backupApkIfNecessary(notAllowedPackages[1], NOT_ALLOWED, any())
+            metadataOutputStream.close()
         }
     }
 

app/src/test/java/com/stevesoltys/seedvault/transport/backup/KVBackupTest.kt

@@ -15,13 +15,14 @@ import io.mockk.coEvery
 import io.mockk.every
 import io.mockk.just
 import io.mockk.mockk
+import io.mockk.verify
 import kotlinx.coroutines.runBlocking
 import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Assertions.assertFalse
 import org.junit.jupiter.api.Assertions.assertTrue
 import org.junit.jupiter.api.Test
 import java.io.IOException
-import java.util.*
+import java.util.Base64
 import kotlin.random.Random
 
 @Suppress("BlockingMethodInNonBlockingContext")
@@ -56,7 +57,10 @@ internal class KVBackupTest : BackupTest() {
     fun `incremental backup with no data gets rejected`() = runBlocking {
         coEvery { plugin.hasDataForPackage(packageInfo) } returns false
 
-        assertEquals(TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED, backup.performBackup(packageInfo, data, FLAG_INCREMENTAL))
+        assertEquals(
+            TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED,
+            backup.performBackup(packageInfo, data, FLAG_INCREMENTAL)
+        )
         assertFalse(backup.hasState())
     }
 
@@ -80,11 +84,15 @@ internal class KVBackupTest : BackupTest() {
     }
 
     @Test
-    fun `ignoring exception when clearing data when non-incremental backup has data`() = runBlocking {
+    fun `ignoring exception when clearing data when non-incremental backup has data`() =
+        runBlocking {
             singleRecordBackup(true)
             coEvery { plugin.removeDataOfPackage(packageInfo) } throws IOException()
 
-        assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, FLAG_NON_INCREMENTAL))
+            assertEquals(
+                TRANSPORT_OK,
+                backup.performBackup(packageInfo, data, FLAG_NON_INCREMENTAL)
+            )
             assertTrue(backup.hasState())
             assertEquals(TRANSPORT_OK, backup.finishBackup())
             assertFalse(backup.hasState())
@@ -139,9 +147,12 @@ internal class KVBackupTest : BackupTest() {
         getDataInput(listOf(true))
         coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream
         every { headerWriter.writeVersion(outputStream, versionHeader) } throws IOException()
+        every { outputStream.close() } just Runs
 
         assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
         assertFalse(backup.hasState())
+
+        verify { outputStream.close() }
     }
 
     @Test
@@ -152,9 +163,12 @@ internal class KVBackupTest : BackupTest() {
         coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream
         every { headerWriter.writeVersion(outputStream, versionHeader) } just Runs
         every { crypto.encryptMultipleSegments(outputStream, any()) } throws IOException()
+        every { outputStream.close() } just Runs
 
         assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
         assertFalse(backup.hasState())
+
+        verify { outputStream.close() }
     }
 
     @Test
@@ -164,9 +178,12 @@ internal class KVBackupTest : BackupTest() {
         writeHeaderAndEncrypt()
         every { outputStream.write(value) } just Runs
         every { outputStream.flush() } throws IOException()
+        every { outputStream.close() } just Runs
 
         assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
         assertFalse(backup.hasState())
+
+        verify { outputStream.close() }
     }
 
     @Test