Cache folder contents in K/V backup/restore

This speeds up things significantly and was needed due to poor
performance of call log backup.

app/src/androidTest/java/com/stevesoltys/seedvault/PluginTest.kt

@@ -202,7 +202,6 @@ class PluginTest : KoinComponent {
         val record3 = Pair(getRandomBase64(128), getRandomByteArray(5 * 1024 * 1024))
 
         // write first record
-        kvBackup.ensureRecordStorageForPackage(packageInfo)
         kvBackup.getOutputStreamForRecord(packageInfo, record1.first).writeAndClose(record1.second)
 
         // data is now available for current token and given package, but not for different token
@@ -220,11 +219,11 @@ class PluginTest : KoinComponent {
         )
 
         // write second and third record
-        kvBackup.ensureRecordStorageForPackage(packageInfo)
         kvBackup.getOutputStreamForRecord(packageInfo, record2.first).writeAndClose(record2.second)
         kvBackup.getOutputStreamForRecord(packageInfo, record3.first).writeAndClose(record3.second)
 
         // all records for package are found and returned properly
+        assertTrue(kvRestore.hasDataForPackage(token, packageInfo))
         records = kvRestore.listRecords(token, packageInfo)
         assertEquals(listOf(record1.first, record2.first, record3.first).sorted(), records.sorted())
         assertReadEquals(
@@ -242,6 +241,7 @@ class PluginTest : KoinComponent {
 
         // delete record3 and ensure that the other two are still found
         kvBackup.deleteRecord(packageInfo, record3.first)
+        assertTrue(kvRestore.hasDataForPackage(token, packageInfo))
         records = kvRestore.listRecords(token, packageInfo)
         assertEquals(listOf(record1.first, record2.first).sorted(), records.sorted())
 
@@ -259,6 +259,7 @@ class PluginTest : KoinComponent {
 
         // initialize storage with given token
         initStorage(token)
+        assertFalse(kvBackup.hasDataForPackage(packageInfo))
 
         // FIXME get Nextcloud to have the same limit
         //  Since Nextcloud is using WebDAV and that seems to have undefined lower file name limits
@@ -271,7 +272,6 @@ class PluginTest : KoinComponent {
         val recordOver = Pair(getRandomBase64(maxOver), getRandomByteArray(1024))
 
         // write max record
-        kvBackup.ensureRecordStorageForPackage(packageInfo)
         kvBackup.getOutputStreamForRecord(packageInfo, recordMax.first)
             .writeAndClose(recordMax.second)
 
@@ -281,7 +281,6 @@ class PluginTest : KoinComponent {
         assertEquals(listOf(recordMax.first), records)
 
         // write exceeding key length record
-        kvBackup.ensureRecordStorageForPackage(packageInfo)
         if (isNextcloud()) {
             // Nextcloud simply refuses to write long filenames
             coAssertThrows(IOException::class.java) {

app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVBackup.kt

@@ -19,39 +19,21 @@ internal class DocumentsProviderKVBackup(
 ) : KVBackupPlugin {
 
     private var packageFile: DocumentFile? = null
+    private var packageChildren: List<DocumentFile>? = null
 
     override fun getQuota(): Long = DEFAULT_QUOTA_KEY_VALUE_BACKUP
 
     @Throws(IOException::class)
     override suspend fun hasDataForPackage(packageInfo: PackageInfo): Boolean {
-        val packageFile =
+        // get the folder for the package (or create it) and all files in it
-            storage.currentKvBackupDir?.findFileBlocking(context, packageInfo.packageName)
+        val dir =
-                ?: return false
-        return packageFile.listFilesBlocking(context).isNotEmpty()
-    }
-
-    @Throws(IOException::class)
-    override suspend fun ensureRecordStorageForPackage(packageInfo: PackageInfo) {
-        // remember package file for subsequent operations
-        packageFile =
             storage.getOrCreateKVBackupDir().createOrGetDirectory(context, packageInfo.packageName)
-    }
+        val children = dir.listFilesBlocking(context)
-
+        // cache package file for subsequent operations
-    @Throws(IOException::class)
+        packageFile = dir
-    override suspend fun removeDataOfPackage(packageInfo: PackageInfo) {
+        // also cache children as doing this for every record is super slow
-        // we cannot use the cached this.packageFile here,
+        packageChildren = children
-        // because this can be called before [ensureRecordStorageForPackage]
+        return children.isNotEmpty()
-        val packageFile =
-            storage.currentKvBackupDir?.findFileBlocking(context, packageInfo.packageName) ?: return
-        packageFile.delete()
-    }
-
-    @Throws(IOException::class)
-    override suspend fun deleteRecord(packageInfo: PackageInfo, key: String) {
-        val packageFile = this.packageFile ?: throw AssertionError()
-        packageFile.assertRightFile(packageInfo)
-        val keyFile = packageFile.findFileBlocking(context, key) ?: return
-        keyFile.delete()
     }
 
     @Throws(IOException::class)
@@ -59,6 +41,7 @@ internal class DocumentsProviderKVBackup(
         packageInfo: PackageInfo,
         key: String
     ): OutputStream {
+        // check maximum key lengths
         check(key.length <= MAX_KEY_LENGTH) {
             "Key $key for ${packageInfo.packageName} is too long: ${key.length} chars."
         }
@@ -68,10 +51,55 @@ internal class DocumentsProviderKVBackup(
                 "Key $key for ${packageInfo.packageName} is too long: ${key.length} chars."
             )
         }
-        val packageFile = this.packageFile ?: throw AssertionError()
+        // get dir and children from cache
+        val packageFile = this.packageFile
+            ?: throw AssertionError("No cached packageFile for ${packageInfo.packageName}")
         packageFile.assertRightFile(packageInfo)
-        val keyFile = packageFile.createOrGetFile(context, key)
+        val children = packageChildren
+            ?: throw AssertionError("No cached children for ${packageInfo.packageName}")
+
+        // get file for key from cache,
+        val keyFile = children.find { it.name == key } // try cache first
+            ?: packageFile.createFile(MIME_TYPE, key) // assume it doesn't exist, create it
+            ?: packageFile.createOrGetFile(context, key) // cache was stale, so try to find it
+        check(keyFile.name == key) { "Key file named ${keyFile.name}, but should be $key" }
         return storage.getOutputStream(keyFile)
     }
 
+    @Throws(IOException::class)
+    override suspend fun deleteRecord(packageInfo: PackageInfo, key: String) {
+        val packageFile = this.packageFile
+            ?: throw AssertionError("No cached packageFile for ${packageInfo.packageName}")
+        packageFile.assertRightFile(packageInfo)
+
+        val children = packageChildren
+            ?: throw AssertionError("No cached children for ${packageInfo.packageName}")
+
+        // try to find file for given key and delete it if found
+        val keyFile = children.find { it.name == key } // try to find in cache
+            ?: packageFile.findFileBlocking(context, key) // fall-back to provider
+            ?: return // not found, nothing left to do
+        keyFile.delete()
+
+        // we don't update the children cache as deleted records
+        // are not expected to get re-added in the same backup pass
+    }
+
+    @Throws(IOException::class)
+    override suspend fun removeDataOfPackage(packageInfo: PackageInfo) {
+        val packageFile = this.packageFile
+            ?: throw AssertionError("No cached packageFile for ${packageInfo.packageName}")
+        packageFile.assertRightFile(packageInfo)
+        // We are not using the cached children here in case they are stale.
+        // This operation isn't frequent, so we don't need to heavily optimize it.
+        packageFile.deleteContents(context)
+        // clear children cache
+        packageChildren = ArrayList()
+    }
+
+    override fun packageFinished(packageInfo: PackageInfo) {
+        packageFile = null
+        packageChildren = null
+    }
+
 }

app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsProviderKVRestorePlugin.kt

@@ -14,13 +14,19 @@ internal class DocumentsProviderKVRestorePlugin(
 ) : KVRestorePlugin {
 
     private var packageDir: DocumentFile? = null
+    private var packageChildren: List<DocumentFile>? = null
 
     override suspend fun hasDataForPackage(token: Long, packageInfo: PackageInfo): Boolean {
         return try {
             val backupDir = storage.getKVBackupDir(token) ?: return false
+            val dir = backupDir.findFileBlocking(context, packageInfo.packageName) ?: return false
+            val children = dir.listFilesBlocking(context)
             // remember package file for subsequent operations
-            packageDir = backupDir.findFileBlocking(context, packageInfo.packageName)
+            packageDir = dir
-            packageDir != null
+            // remember package children for subsequent operations
+            packageChildren = children
+            // we have data if we have a non-empty list of children
+            children.isNotEmpty()
         } catch (e: IOException) {
             false
         }
@@ -28,11 +34,13 @@ internal class DocumentsProviderKVRestorePlugin(
 
     @Throws(IOException::class)
     override suspend fun listRecords(token: Long, packageInfo: PackageInfo): List<String> {
-        val packageDir = this.packageDir ?: throw AssertionError()
+        val packageDir = this.packageDir
+            ?: throw AssertionError("No cached packageDir for ${packageInfo.packageName}")
         packageDir.assertRightFile(packageInfo)
-        return packageDir.listFilesBlocking(context)
+        return packageChildren
-            .filter { file -> file.name != null }
+            ?.filter { file -> file.name != null }
-            .map { file -> file.name!! }
+            ?.map { file -> file.name!! }
+            ?: throw AssertionError("No cached children for ${packageInfo.packageName}")
     }
 
     @Throws(IOException::class)
@@ -41,9 +49,12 @@ internal class DocumentsProviderKVRestorePlugin(
         packageInfo: PackageInfo,
         key: String
     ): InputStream {
-        val packageDir = this.packageDir ?: throw AssertionError()
+        val packageDir = this.packageDir
+            ?: throw AssertionError("No cached packageDir for ${packageInfo.packageName}")
         packageDir.assertRightFile(packageInfo)
-        val keyFile = packageDir.findFileBlocking(context, key) ?: throw IOException()
+        val keyFile = packageChildren?.find { it.name == key }
+            ?: packageDir.findFileBlocking(context, key)
+            ?: throw IOException()
         return storage.getInputStream(keyFile)
     }
 

app/src/main/java/com/stevesoltys/seedvault/plugins/saf/DocumentsStorage.kt

@@ -32,7 +32,7 @@ const val DIRECTORY_FULL_BACKUP = "full"
 const val DIRECTORY_KEY_VALUE_BACKUP = "kv"
 const val FILE_BACKUP_METADATA = ".backup.metadata"
 const val FILE_NO_MEDIA = ".nomedia"
-private const val MIME_TYPE = "application/octet-stream"
+const val MIME_TYPE = "application/octet-stream"
 
 private val TAG = DocumentsStorage::class.java.simpleName
 

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

@@ -91,14 +91,6 @@ internal class KVBackup(
             }
         }
 
-        // ensure there's a place to store K/V for the given package
-        try {
-            plugin.ensureRecordStorageForPackage(packageInfo)
-        } catch (e: IOException) {
-            Log.e(TAG, "Error ensuring storage for ${packageInfo.packageName}.", e)
-            return backupError(TRANSPORT_ERROR)
-        }
-
         // parse and store the K/V updates
         return storeRecords(packageInfo, data)
     }
@@ -221,6 +213,7 @@ internal class KVBackup(
 
     fun finishBackup(): Int {
         Log.i(TAG, "Finish K/V Backup of ${state!!.packageInfo.packageName}")
+        plugin.packageFinished(state!!.packageInfo)
         state = null
         return TRANSPORT_OK
     }
@@ -233,6 +226,7 @@ internal class KVBackup(
         "Resetting state because of K/V Backup error of ${state!!.packageInfo.packageName}".let {
             Log.i(TAG, it)
         }
+        plugin.packageFinished(state!!.packageInfo)
         state = null
         return result
     }

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

@@ -14,18 +14,13 @@ interface KVBackupPlugin {
     // TODO consider using a salted hash for the package name (and key) to not leak it to the storage server
     /**
      * Return true if there are records stored for the given package.
-     */
+     * This is always called first per [PackageInfo], before subsequent methods.
-    @Throws(IOException::class)
-    suspend fun hasDataForPackage(packageInfo: PackageInfo): Boolean
-
-    /**
-     * This marks the beginning of a backup operation.
      *
-     * Make sure that there is a place to store K/V pairs for the given package.
+     * Independent of the return value, the storage should now be prepared to store K/V pairs.
      * E.g. file-based plugins should a create a directory for the package, if none exists.
      */
     @Throws(IOException::class)
-    suspend fun ensureRecordStorageForPackage(packageInfo: PackageInfo)
+    suspend fun hasDataForPackage(packageInfo: PackageInfo): Boolean
 
     /**
      * Return an [OutputStream] for the given package and key
@@ -41,9 +36,16 @@ interface KVBackupPlugin {
     suspend fun deleteRecord(packageInfo: PackageInfo, key: String)
 
     /**
-     * Remove all data associated with the given package.
+     * Remove all data associated with the given package,
+     * but be prepared to receive new records afterwards with [getOutputStreamForRecord].
      */
     @Throws(IOException::class)
     suspend fun removeDataOfPackage(packageInfo: PackageInfo)
 
+    /**
+     * The package finished backup.
+     * This can be an opportunity to clear existing caches or to do other clean-up work.
+     */
+    fun packageFinished(packageInfo: PackageInfo)
+
 }

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

@@ -15,7 +15,8 @@ interface KVRestorePlugin {
     /**
      * Return all record keys for the given token and package.
      *
-     * Note: Implementations might expect that you call [hasDataForPackage] before.
+     * Note: Implementations usually expect that you call [hasDataForPackage]
+     *       with the same parameters before.
      *
      * For file-based plugins, this is usually a list of file names in the package directory.
      */

app/src/test/java/com/stevesoltys/seedvault/transport/CoordinatorIntegrationTest.kt

@@ -126,7 +126,6 @@ internal class CoordinatorIntegrationTest : TransportTest() {
 
         // read one key/value record and write it to output stream
         coEvery { kvBackupPlugin.hasDataForPackage(packageInfo) } returns false
-        coEvery { kvBackupPlugin.ensureRecordStorageForPackage(packageInfo) } just Runs
         every { inputFactory.getBackupDataInput(fileDescriptor) } returns backupDataInput
         every { backupDataInput.readNextHeader() } returns true andThen true andThen false
         every { backupDataInput.key } returns key andThen key2
@@ -151,6 +150,7 @@ internal class CoordinatorIntegrationTest : TransportTest() {
                 key264
             )
         } returns bOutputStream2
+        every { kvBackupPlugin.packageFinished(packageInfo) } just Runs
         coEvery {
             apkBackup.backupApkIfNecessary(
                 packageInfo,
@@ -219,7 +219,6 @@ internal class CoordinatorIntegrationTest : TransportTest() {
 
         // read one key/value record and write it to output stream
         coEvery { kvBackupPlugin.hasDataForPackage(packageInfo) } returns false
-        coEvery { kvBackupPlugin.ensureRecordStorageForPackage(packageInfo) } just Runs
         every { inputFactory.getBackupDataInput(fileDescriptor) } returns backupDataInput
         every { backupDataInput.readNextHeader() } returns true andThen false
         every { backupDataInput.key } returns key
@@ -234,6 +233,7 @@ internal class CoordinatorIntegrationTest : TransportTest() {
                 key64
             )
         } returns bOutputStream
+        every { kvBackupPlugin.packageFinished(packageInfo) } just Runs
         coEvery { apkBackup.backupApkIfNecessary(packageInfo, UNKNOWN_ERROR, any()) } returns null
         coEvery { backupPlugin.getMetadataOutputStream() } returns metadataOutputStream
         every { metadataManager.onPackageBackedUp(packageInfo, metadataOutputStream) } just Runs

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

@@ -90,6 +90,9 @@ internal class KVBackupTest : BackupTest() {
 
         assertEquals(TRANSPORT_OK, backup.performBackup(pmPackageInfo, data, 0))
         assertTrue(backup.hasState())
+
+        every { plugin.packageFinished(pmPackageInfo) } just Runs
+
         assertEquals(TRANSPORT_OK, backup.finishBackup())
         assertFalse(backup.hasState())
 
@@ -103,6 +106,7 @@ internal class KVBackupTest : BackupTest() {
     @Test
     fun `incremental backup with no data gets rejected`() = runBlocking {
         coEvery { plugin.hasDataForPackage(packageInfo) } returns false
+        every { plugin.packageFinished(packageInfo) } just Runs
 
         assertEquals(
             TRANSPORT_NON_INCREMENTAL_BACKUP_REQUIRED,
@@ -114,6 +118,7 @@ internal class KVBackupTest : BackupTest() {
     @Test
     fun `check for existing data throws exception`() = runBlocking {
         coEvery { plugin.hasDataForPackage(packageInfo) } throws IOException()
+        every { plugin.packageFinished(packageInfo) } just Runs
 
         assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
         assertFalse(backup.hasState())
@@ -145,20 +150,12 @@ internal class KVBackupTest : BackupTest() {
             assertFalse(backup.hasState())
         }
 
-    @Test
-    fun `ensuring storage throws exception`() = runBlocking {
-        coEvery { plugin.hasDataForPackage(packageInfo) } returns false
-        coEvery { plugin.ensureRecordStorageForPackage(packageInfo) } throws IOException()
-
-        assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
-        assertFalse(backup.hasState())
-    }
-
     @Test
     fun `exception while reading next header`() = runBlocking {
         initPlugin(false)
         createBackupDataInput()
         every { dataInput.readNextHeader() } throws IOException()
+        every { plugin.packageFinished(packageInfo) } just Runs
 
         assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
         assertFalse(backup.hasState())
@@ -172,6 +169,7 @@ internal class KVBackupTest : BackupTest() {
         every { dataInput.key } returns key
         every { dataInput.dataSize } returns value.size
         every { dataInput.readEntityData(any(), 0, value.size) } throws IOException()
+        every { plugin.packageFinished(packageInfo) } just Runs
 
         assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
         assertFalse(backup.hasState())
@@ -181,6 +179,7 @@ internal class KVBackupTest : BackupTest() {
     fun `no data records`() = runBlocking {
         initPlugin(false)
         getDataInput(listOf(false))
+        every { plugin.packageFinished(packageInfo) } just Runs
 
         assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, 0))
         assertTrue(backup.hasState())
@@ -195,6 +194,7 @@ internal class KVBackupTest : BackupTest() {
         coEvery { plugin.getOutputStreamForRecord(packageInfo, key64) } returns outputStream
         every { headerWriter.writeVersion(outputStream, versionHeader) } throws IOException()
         every { outputStream.close() } just Runs
+        every { plugin.packageFinished(packageInfo) } just Runs
 
         assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
         assertFalse(backup.hasState())
@@ -211,6 +211,7 @@ internal class KVBackupTest : BackupTest() {
         every { headerWriter.writeVersion(outputStream, versionHeader) } just Runs
         every { crypto.encryptMultipleSegments(outputStream, any()) } throws IOException()
         every { outputStream.close() } just Runs
+        every { plugin.packageFinished(packageInfo) } just Runs
 
         assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
         assertFalse(backup.hasState())
@@ -226,6 +227,7 @@ internal class KVBackupTest : BackupTest() {
         every { outputStream.write(value) } just Runs
         every { outputStream.flush() } throws IOException()
         every { outputStream.close() } just Runs
+        every { plugin.packageFinished(packageInfo) } just Runs
 
         assertEquals(TRANSPORT_ERROR, backup.performBackup(packageInfo, data, 0))
         assertFalse(backup.hasState())
@@ -241,6 +243,7 @@ internal class KVBackupTest : BackupTest() {
         every { outputStream.write(value) } just Runs
         every { outputStream.flush() } just Runs
         every { outputStream.close() } throws IOException()
+        every { plugin.packageFinished(packageInfo) } just Runs
 
         assertEquals(TRANSPORT_OK, backup.performBackup(packageInfo, data, 0))
         assertTrue(backup.hasState())
@@ -255,11 +258,11 @@ internal class KVBackupTest : BackupTest() {
         every { outputStream.write(value) } just Runs
         every { outputStream.flush() } just Runs
         every { outputStream.close() } just Runs
+        every { plugin.packageFinished(packageInfo) } just Runs
     }
 
     private fun initPlugin(hasDataForPackage: Boolean = false, pi: PackageInfo = packageInfo) {
         coEvery { plugin.hasDataForPackage(pi) } returns hasDataForPackage
-        coEvery { plugin.ensureRecordStorageForPackage(pi) } just Runs
     }
 
     private fun createBackupDataInput() {