Fix issue with DocumentFileCache

app/src/androidTest/java/com/stevesoltys/seedvault/backend/saf/SafBackendTest.kt

@@ -25,21 +25,23 @@ class SafBackendTest : BackendTest(), KoinComponent {
 
     private val context = InstrumentationRegistry.getInstrumentation().targetContext
     private val settingsManager by inject<SettingsManager>()
-    override val plugin: Backend
+    private val safStorage = settingsManager.getSafProperties() ?: error("No SAF storage")
-        get() {
+    private val safProperties = SafProperties(
-            val safStorage = settingsManager.getSafProperties() ?: error("No SAF storage")
+        config = safStorage.config,
-            val safProperties = SafProperties(
+        name = safStorage.name,
-                config = safStorage.config,
+        isUsb = safStorage.isUsb,
-                name = safStorage.name,
+        requiresNetwork = safStorage.requiresNetwork,
-                isUsb = safStorage.isUsb,
+        rootId = safStorage.rootId,
-                requiresNetwork = safStorage.requiresNetwork,
+    )
-                rootId = safStorage.rootId,
+    override val plugin: Backend = SafBackend(context, safProperties, ".SeedvaultTest")
-            )
-            return SafBackend(context, safProperties, ".SeedvaultTest")
-        }
 
     @Test
-    fun test(): Unit = runBlocking {
+    fun `test write list read rename delete`(): Unit = runBlocking {
         testWriteListReadRenameDelete()
     }
+
+    @Test
+    fun `test remove create write file`(): Unit = runBlocking {
+        testRemoveCreateWriteFile()
+    }
 }

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

@@ -15,11 +15,11 @@ import android.content.pm.PackageInfo
 import android.os.ParcelFileDescriptor
 import android.util.Log
 import com.stevesoltys.seedvault.MAGIC_PACKAGE_MANAGER
+import com.stevesoltys.seedvault.backend.BackendManager
+import com.stevesoltys.seedvault.backend.isOutOfSpace
 import com.stevesoltys.seedvault.crypto.Crypto
 import com.stevesoltys.seedvault.header.VERSION
 import com.stevesoltys.seedvault.header.getADForKV
-import com.stevesoltys.seedvault.backend.BackendManager
-import com.stevesoltys.seedvault.backend.isOutOfSpace
 import com.stevesoltys.seedvault.settings.SettingsManager
 import com.stevesoltys.seedvault.ui.notification.BackupNotificationManager
 import org.calyxos.seedvault.core.backends.LegacyAppBackupFile

app/src/main/resources/simplelogger.properties

@@ -0,0 +1 @@
+org.slf4j.simpleLogger.defaultLogLevel=debug

core/src/main/java/org/calyxos/seedvault/core/backends/BackendTest.kt

@@ -6,14 +6,12 @@
 package org.calyxos.seedvault.core.backends
 
 import androidx.annotation.VisibleForTesting
-import at.bitfire.dav4jvm.exception.HttpException
 import org.calyxos.seedvault.core.toHexString
-import org.junit.Assert.assertArrayEquals
 import kotlin.random.Random
+import kotlin.test.assertContentEquals
 import kotlin.test.assertEquals
 import kotlin.test.assertFailsWith
 import kotlin.test.assertNotNull
-import kotlin.test.fail
 
 @VisibleForTesting
 public abstract class BackendTest {
@@ -21,12 +19,9 @@ public abstract class BackendTest {
     public abstract val plugin: Backend
 
     protected suspend fun testWriteListReadRenameDelete() {
-        try {
+        plugin.removeAll()
-            plugin.removeAll()
-        } catch (e: HttpException) {
-            if (e.code != 404) fail(e.message, e)
-        }
 
+        val androidId = "0123456789abcdef"
         val now = System.currentTimeMillis()
         val bytes1 = Random.nextBytes(1337)
         val bytes2 = Random.nextBytes(1337 * 8)
@@ -34,7 +29,7 @@ public abstract class BackendTest {
             it.write(bytes1)
         }
 
-        plugin.save(FileBackupFileType.Snapshot("0123456789abcdef", now)).use {
+        plugin.save(FileBackupFileType.Snapshot(androidId, now)).use {
             it.write(bytes2)
         }
 
@@ -56,13 +51,13 @@ public abstract class BackendTest {
         assertNotNull(metadata)
         assertNotNull(snapshot)
 
-        assertArrayEquals(bytes1, plugin.load(metadata as FileHandle).readAllBytes())
+        assertContentEquals(bytes1, plugin.load(metadata as FileHandle).readAllBytes())
-        assertArrayEquals(bytes2, plugin.load(snapshot as FileHandle).readAllBytes())
+        assertContentEquals(bytes2, plugin.load(snapshot as FileHandle).readAllBytes())
 
         val blobName = Random.nextBytes(32).toHexString()
         var blob: FileBackupFileType.Blob? = null
         val bytes3 = Random.nextBytes(1337 * 16)
-        plugin.save(FileBackupFileType.Blob("0123456789abcdef", blobName)).use {
+        plugin.save(FileBackupFileType.Blob(androidId, blobName)).use {
             it.write(bytes3)
         }
         plugin.list(
@@ -77,7 +72,7 @@ public abstract class BackendTest {
             }
         }
         assertNotNull(blob)
-        assertArrayEquals(bytes3, plugin.load(blob as FileHandle).readAllBytes())
+        assertContentEquals(bytes3, plugin.load(blob as FileHandle).readAllBytes())
 
         // try listing with top-level folder, should find two files of FileBackupFileType in there
         var numFiles = 0
@@ -92,7 +87,7 @@ public abstract class BackendTest {
         plugin.remove(snapshot as FileHandle)
 
         // rename snapshots
-        val snapshotNewFolder = TopLevelFolder("0123456789abcdee.sv")
+        val snapshotNewFolder = TopLevelFolder("a123456789abcdef.sv")
         plugin.rename(snapshot!!.topLevelFolder, snapshotNewFolder)
 
         // rename to existing folder should fail
@@ -105,4 +100,20 @@ public abstract class BackendTest {
         plugin.remove(snapshotNewFolder)
     }
 
+    protected suspend fun testRemoveCreateWriteFile() {
+        val now = System.currentTimeMillis()
+        val blob = LegacyAppBackupFile.Blob(now, Random.nextBytes(32).toHexString())
+        val bytes = Random.nextBytes(2342)
+
+        plugin.remove(blob)
+        try {
+            plugin.save(blob).use {
+                it.write(bytes)
+            }
+            assertContentEquals(bytes, plugin.load(blob as FileHandle).readAllBytes())
+        } finally {
+            plugin.remove(blob)
+        }
+    }
+
 }

core/src/main/java/org/calyxos/seedvault/core/backends/saf/DocumentFileCache.kt

@@ -11,6 +11,7 @@ import org.calyxos.seedvault.core.backends.FileBackupFileType
 import org.calyxos.seedvault.core.backends.FileHandle
 import org.calyxos.seedvault.core.backends.LegacyAppBackupFile
 import org.calyxos.seedvault.core.backends.TopLevelFolder
+import java.util.concurrent.ConcurrentHashMap
 
 internal class DocumentFileCache(
     private val context: Context,
@@ -18,7 +19,7 @@ internal class DocumentFileCache(
     private val root: String,
 ) {
 
-    private val cache = mutableMapOf<String, DocumentFile>()
+    private val cache = ConcurrentHashMap<String, DocumentFile>()
 
     internal suspend fun getRootFile(): DocumentFile {
         return cache.getOrPut(root) {
@@ -26,24 +27,53 @@ internal class DocumentFileCache(
         }
     }
 
-    internal suspend fun getFile(fh: FileHandle): DocumentFile = when (fh) {
+    internal suspend fun getOrCreateFile(fh: FileHandle): DocumentFile = when (fh) {
         is TopLevelFolder -> cache.getOrPut("$root/${fh.relativePath}") {
             getRootFile().getOrCreateDirectory(context, fh.name)
         }
 
         is LegacyAppBackupFile -> cache.getOrPut("$root/${fh.relativePath}") {
-            getFile(fh.topLevelFolder).getOrCreateFile(context, fh.name)
+                getOrCreateFile(fh.topLevelFolder).getOrCreateFile(context, fh.name)
-        }
+            }
 
         is FileBackupFileType.Blob -> {
             val subFolderName = fh.name.substring(0, 2)
             cache.getOrPut("$root/${fh.topLevelFolder.name}/$subFolderName") {
-                getFile(fh.topLevelFolder).getOrCreateDirectory(context, subFolderName)
+                getOrCreateFile(fh.topLevelFolder).getOrCreateDirectory(context, subFolderName)
             }.getOrCreateFile(context, fh.name)
         }
 
         is FileBackupFileType.Snapshot -> {
-            getFile(fh.topLevelFolder).getOrCreateFile(context, fh.name)
+            getOrCreateFile(fh.topLevelFolder).getOrCreateFile(context, fh.name)
         }
     }
+
+    internal suspend fun getFile(fh: FileHandle): DocumentFile? = when (fh) {
+        is TopLevelFolder -> cache.getOrElse("$root/${fh.relativePath}") {
+            getRootFile().findFileBlocking(context, fh.name)
+        }
+
+        is LegacyAppBackupFile -> cache.getOrElse("$root/${fh.relativePath}") {
+            getFile(fh.topLevelFolder)?.findFileBlocking(context, fh.name)
+        }
+
+        is FileBackupFileType.Blob -> {
+            val subFolderName = fh.name.substring(0, 2)
+            cache.getOrElse("$root/${fh.topLevelFolder.name}/$subFolderName") {
+                getFile(fh.topLevelFolder)?.findFileBlocking(context, subFolderName)
+            }?.findFileBlocking(context, fh.name)
+        }
+
+        is FileBackupFileType.Snapshot -> {
+            getFile(fh.topLevelFolder)?.findFileBlocking(context, fh.name)
+        }
+    }
+
+    internal fun removeFromCache(fh: FileHandle) {
+        cache.remove("$root/${fh.relativePath}")
+    }
+
+    internal fun clearAll() {
+        cache.clear()
+    }
 }

core/src/main/java/org/calyxos/seedvault/core/backends/saf/SafBackend.kt

@@ -14,6 +14,7 @@ import android.provider.DocumentsContract.Root.COLUMN_ROOT_ID
 import android.provider.DocumentsContract.renameDocument
 import androidx.core.database.getIntOrNull
 import androidx.documentfile.provider.DocumentFile
+import io.github.oshai.kotlinlogging.KLogger
 import io.github.oshai.kotlinlogging.KotlinLogging
 import org.calyxos.seedvault.core.backends.Backend
 import org.calyxos.seedvault.core.backends.Constants.DIRECTORY_ROOT
@@ -37,6 +38,8 @@ import kotlin.reflect.KClass
 internal const val AUTHORITY_STORAGE = "com.android.externalstorage.documents"
 internal const val ROOT_ID_DEVICE = "primary"
 
+private const val DEBUG_LOG = true
+
 public class SafBackend(
     private val appContext: Context,
     private val safProperties: SafProperties,
@@ -52,10 +55,12 @@ public class SafBackend(
     private val cache = DocumentFileCache(context, safProperties.getDocumentFile(context), root)
 
     override suspend fun test(): Boolean {
+        log.debugLog { "test()" }
         return cache.getRootFile().isDirectory
     }
 
     override suspend fun getFreeSpace(): Long? {
+        log.debugLog { "getFreeSpace()" }
         val rootId = safProperties.rootId ?: return null
         val authority = safProperties.uri.authority
         // using DocumentsContract#buildRootUri(String, String) with rootId directly doesn't work
@@ -82,12 +87,14 @@ public class SafBackend(
     }
 
     override suspend fun save(handle: FileHandle): OutputStream {
-        val file = cache.getFile(handle)
+        log.debugLog { "save($handle)" }
+        val file = cache.getOrCreateFile(handle)
         return file.getOutputStream(context.contentResolver)
     }
 
     override suspend fun load(handle: FileHandle): InputStream {
-        val file = cache.getFile(handle)
+        log.debugLog { "load($handle)" }
+        val file = cache.getOrCreateFile(handle)
         return file.getInputStream(context.contentResolver)
     }
 
@@ -101,10 +108,12 @@ public class SafBackend(
         if (LegacyAppBackupFile.IconsFile::class in fileTypes) throw UnsupportedOperationException()
         if (LegacyAppBackupFile.Blob::class in fileTypes) throw UnsupportedOperationException()
 
+        log.debugLog { "list($topLevelFolder, $fileTypes)" }
+
         val folder = if (topLevelFolder == null) {
             cache.getRootFile()
         } else {
-            cache.getFile(topLevelFolder)
+            cache.getOrCreateFile(topLevelFolder)
         }
         // limit depth based on wanted types and if top-level folder is given
         var depth = if (FileBackupFileType.Blob::class in fileTypes) 3 else 2
@@ -161,12 +170,16 @@ public class SafBackend(
     }
 
     override suspend fun remove(handle: FileHandle) {
-        val file = cache.getFile(handle)
+        log.debugLog { "remove($handle)" }
-        if (!file.delete()) throw IOException("could not delete ${handle.relativePath}")
+        cache.getFile(handle)?.let { file ->
+            if (!file.delete()) throw IOException("could not delete ${handle.relativePath}")
+            cache.removeFromCache(handle)
+        }
     }
 
     override suspend fun rename(from: TopLevelFolder, to: TopLevelFolder) {
-        val fromFile = cache.getFile(from)
+        log.debugLog { "rename($from, ${to.name})" }
+        val fromFile = cache.getOrCreateFile(from)
         // don't use fromFile.renameTo(to.name) as that creates "${to.name} (1)"
         val newUri = renameDocument(context.contentResolver, fromFile.uri, to.name)
             ?: throw IOException("could not rename ${from.relativePath}")
@@ -179,16 +192,28 @@ public class SafBackend(
     }
 
     override suspend fun removeAll() {
-        cache.getRootFile().listFilesBlocking(context).forEach {
+        log.debugLog { "removeAll()" }
-            it.delete()
+        try {
+            cache.getRootFile().listFilesBlocking(context).forEach { file ->
+                log.debugLog { "  remove ${file.uri}" }
+                file.delete()
+            }
+        } finally {
+            cache.clearAll()
         }
     }
 
     override val providerPackageName: String? by lazy {
+        log.debugLog { "providerPackageName" }
         val authority = safProperties.uri.authority ?: return@lazy null
         val providerInfo = context.packageManager.resolveContentProvider(authority, 0)
             ?: return@lazy null
+        log.debugLog { "  ${providerInfo.packageName}" }
         providerInfo.packageName
     }
 
 }
+
+private inline fun KLogger.debugLog(crossinline block: () -> String) {
+    if (DEBUG_LOG) debug { block() }
+}

core/src/test/java/org/calyxos/seedvault/core/backends/webdav/WebDavBackendTest.kt

@@ -17,4 +17,9 @@ public class WebDavBackendTest : BackendTest() {
     public fun `test write, list, read, rename, delete`(): Unit = runBlocking {
         testWriteListReadRenameDelete()
     }
+
+    @Test
+    public fun `test remove, create, write file`(): Unit = runBlocking {
+        testRemoveCreateWriteFile()
+    }
 }