Only back up APK and write metadata when app was actually backed up

Apps that have nothing to back up start a backup but then get a call to cancelFullBackup()
and never even call finishBackup().
Do not write metadata for such apps, the call got moved to finishBackup().

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

@@ -116,9 +116,7 @@ internal class BackupCoordinator(
         if (packageName == MAGIC_PACKAGE_MANAGER && getBackupBackoff() != 0L) {
             return TRANSPORT_PACKAGE_REJECTED
         }
-
+        return kv.performBackup(packageInfo, data, flags)
-        val result = kv.performBackup(packageInfo, data, flags)
-        return onPackageBackedUp(result, packageInfo)
     }
 
     // ------------------------------------------------------------------------------------
@@ -145,12 +143,24 @@ internal class BackupCoordinator(
     fun checkFullBackupSize(size: Long) = full.checkFullBackupSize(size)
 
     fun performFullBackup(targetPackage: PackageInfo, fileDescriptor: ParcelFileDescriptor, flags: Int): Int {
-        val result = full.performFullBackup(targetPackage, fileDescriptor, flags)
+        return full.performFullBackup(targetPackage, fileDescriptor, flags)
-        return onPackageBackedUp(result, targetPackage)
     }
 
     fun sendBackupData(numBytes: Int) = full.sendBackupData(numBytes)
 
+    /**
+     * Tells the transport to cancel the currently-ongoing full backup operation.
+     * This will happen between [performFullBackup] and [finishBackup]
+     * if the OS needs to abort the backup operation for any reason,
+     * such as a crash in the application undergoing backup.
+     *
+     * When it receives this call,
+     * the transport should discard any partial archive that it has stored so far.
+     * If possible it should also roll back to the previous known-good archive in its data store.
+     *
+     * If the transport receives this callback, it will *not* receive a call to [finishBackup].
+     * It needs to tear down any ongoing backup state here.
+     */
     fun cancelFullBackup() = full.cancelFullBackup()
 
     // Clear and Finish
@@ -186,10 +196,12 @@ internal class BackupCoordinator(
     fun finishBackup(): Int = when {
         kv.hasState() -> {
             check(!full.hasState()) { "K/V backup has state, but full backup has dangling state as well" }
+            onPackageBackedUp(kv.getCurrentPackage()!!)  // not-null because we have state
             kv.finishBackup()
         }
         full.hasState() -> {
             check(!kv.hasState()) { "Full backup has state, but K/V backup has dangling state as well" }
+            onPackageBackedUp(full.getCurrentPackage()!!)  // not-null because we have state
             full.finishBackup()
         }
         calledInitialize || calledClearBackupData -> {
@@ -200,8 +212,7 @@ internal class BackupCoordinator(
         else -> throw IllegalStateException("Unexpected state in finishBackup()")
     }
 
-    private fun onPackageBackedUp(result: Int, packageInfo: PackageInfo): Int {
+    private fun onPackageBackedUp(packageInfo: PackageInfo) {
-        if (result != TRANSPORT_OK) return result
         val packageName = packageInfo.packageName
         try {
             apkBackup.backupApkIfNecessary(packageInfo) { plugin.getApkOutputStream(packageInfo) }
@@ -209,9 +220,7 @@ internal class BackupCoordinator(
             metadataManager.onPackageBackedUp(packageName, outputStream)
         } catch (e: IOException) {
             Log.e(TAG, "Error while writing metadata for $packageName", e)
-            return TRANSPORT_PACKAGE_REJECTED
         }
-        return result
     }
 
     private fun getBackupBackoff(): Long {

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

@@ -36,6 +36,8 @@ internal class FullBackup(
 
     fun hasState() = state != null
 
+    fun getCurrentPackage() = state?.packageInfo
+
     fun getQuota(): Long = plugin.getQuota()
 
     fun checkFullBackupSize(size: Long): Int {
@@ -98,6 +100,9 @@ internal class FullBackup(
         val inputStream = inputFactory.getInputStream(socket)
         state = FullBackupState(targetPackage, socket, inputStream, outputStream)
 
+        // TODO store this is clearable lamdba and only run it when we actually get data
+        //      this is to avoid unnecessary disk I/O
+
         // store version header
         val state = this.state ?: throw AssertionError()
         val header = VersionHeader(packageName = state.packageName)

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

@@ -11,7 +11,7 @@ import com.stevesoltys.seedvault.header.VersionHeader
 import libcore.io.IoUtils.closeQuietly
 import java.io.IOException
 
-class KVBackupState(internal val packageName: String)
+class KVBackupState(internal val packageInfo: PackageInfo)
 
 const val DEFAULT_QUOTA_KEY_VALUE_BACKUP = (2 * (5 * 1024 * 1024)).toLong()
 
@@ -27,6 +27,8 @@ internal class KVBackup(
 
     fun hasState() = state != null
 
+    fun getCurrentPackage() = state?.packageInfo
+
     fun getQuota(): Long = plugin.getQuota()
 
     fun performBackup(packageInfo: PackageInfo, data: ParcelFileDescriptor, flags: Int): Int {
@@ -48,7 +50,7 @@ internal class KVBackup(
 
         // initialize state
         if (this.state != null) throw AssertionError()
-        this.state = KVBackupState(packageInfo.packageName)
+        this.state = KVBackupState(packageInfo)
 
         // check if we have existing data for the given package
         val hasDataForPackage = try {
@@ -162,7 +164,7 @@ internal class KVBackup(
     }
 
     fun finishBackup(): Int {
-        Log.i(TAG, "Finish K/V Backup of ${state!!.packageName}")
+        Log.i(TAG, "Finish K/V Backup of ${state!!.packageInfo.packageName}")
         state = null
         return TRANSPORT_OK
     }
@@ -172,7 +174,7 @@ internal class KVBackup(
      * because [finishBackup] is not called when we don't return [TRANSPORT_OK].
      */
     private fun backupError(result: Int): Int {
-        Log.i(TAG, "Resetting state because of K/V Backup error of ${state!!.packageName}")
+        Log.i(TAG, "Resetting state because of K/V Backup error of ${state!!.packageInfo.packageName}")
         state = null
         return result
     }

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

@@ -142,6 +142,8 @@ internal class BackupCoordinatorTest: BackupTest() {
 
         every { kv.hasState() } returns true
         every { full.hasState() } returns false
+        every { kv.getCurrentPackage() } returns packageInfo
+        expectApkBackupAndMetadataWrite()
         every { kv.finishBackup() } returns result
 
         assertEquals(result, backup.finishBackup())
@@ -153,9 +155,17 @@ internal class BackupCoordinatorTest: BackupTest() {
 
         every { kv.hasState() } returns false
         every { full.hasState() } returns true
+        every { full.getCurrentPackage() } returns packageInfo
+        expectApkBackupAndMetadataWrite()
         every { full.finishBackup() } returns result
 
         assertEquals(result, backup.finishBackup())
     }
 
+    private fun expectApkBackupAndMetadataWrite() {
+        every { apkBackup.backupApkIfNecessary(packageInfo, any()) } returns true
+        every { plugin.getMetadataOutputStream() } returns metadataOutputStream
+        every { metadataManager.onPackageBackedUp(packageInfo.packageName, metadataOutputStream) } just Runs
+    }
+
 }