From d7716eed98b59cfb3deb4081d177eda048106c35 Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Tue, 1 Sep 2026 21:39:29 +0200 Subject: [PATCH 1/8] fix(android): guard recovered writeback mutations --- ...AndroidVirtualFileCacheInstrumentedTest.kt | 29 ++ .../AndroidDocumentWritebackRecovery.kt | 282 ++++++++++++++++++ .../NextcloudDocumentsProvider.kt | 263 +--------------- .../AndroidVirtualFileProxyCallbackTest.kt | 25 ++ 4 files changed, 339 insertions(+), 260 deletions(-) create mode 100644 androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt diff --git a/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt index 2ebcac551..92cb4bc46 100644 --- a/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt +++ b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt @@ -128,6 +128,35 @@ class AndroidVirtualFileCacheInstrumentedTest { assertEquals(0, androidDocumentPendingWritebackCount(context, session)) } + @Test + fun processRestoredWritebackBlocksDestructiveMutationUntilRecovery() { + val recovery = File(context.filesDir, "documents-recovery").apply { mkdirs() } + val stage = File(recovery, "writeback-restored.stage").apply { writeText("local edit") } + File(recovery, stage.name + ".json").writeText( + JSONObject() + .put("version", 1) + .put("account", NextcloudDocumentIds.accountKey(session)) + .put("path", "Projects/Active/notes.txt") + .put("etag", "\"v1\"") + .put("displayName", "notes.txt") + .put("stage", stage.name) + .put("startedAt", 10L) + .put("ready", true) + .toString(), + ) + + org.junit.Assert.assertThrows(IllegalStateException::class.java) { + withNoBlockingAndroidDocumentWriteback(context, session, "Projects/Active") { + error("The blocked mutation must not run.") + } + } + var unrelatedMutationRan = false + withNoBlockingAndroidDocumentWriteback(context, session, "Projects/Archive") { + unrelatedMutationRan = true + } + org.junit.Assert.assertTrue(unrelatedMutationRan) + } + @Test fun providerStartupDiscardsIncompleteWritebacksAndKeepsReadyRecovery() { val recovery = File(context.filesDir, "documents-recovery").apply { mkdirs() } diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt new file mode 100644 index 000000000..05e8e6d07 --- /dev/null +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt @@ -0,0 +1,282 @@ +package dev.obiente.nextcloudnative + +import dev.obiente.nextcloudnative.app.NextcloudSession +import java.io.File +import java.io.FileOutputStream +import java.nio.file.AtomicMoveNotSupportedException +import java.nio.file.Files +import java.nio.file.StandardCopyOption +import java.util.concurrent.ConcurrentHashMap +import org.json.JSONObject + +internal const val MAX_ANDROID_DOCUMENT_WRITEBACK_BYTES = Long.MAX_VALUE +internal const val MIN_ANDROID_DOCUMENT_FREE_BYTES = 512L * 1024L * 1024L + +internal fun requireAndroidDocumentWritebackCapacity(remoteSize: Long, availableBytes: Long) { + require(remoteSize >= 0L && availableBytes >= 0L) + require(remoteSize <= (availableBytes - MIN_ANDROID_DOCUMENT_FREE_BYTES).coerceAtLeast(0L)) { + "There is not enough free space to stage this edit safely." + } +} + +internal fun requireAndroidDocumentStagedWritebackCapacity(stagedBytes: Long, availableBytes: Long) { + require(stagedBytes >= 0L && availableBytes >= 0L) + require(availableBytes >= MIN_ANDROID_DOCUMENT_FREE_BYTES) { + "There is not enough free space to retain this edit safely." + } +} + +internal data class AndroidDocumentPendingWriteback( + val staging: File, + val manifest: File, + val accountId: String, + val remotePath: String, + val expectedRemoteEtag: String, + val conflict: Boolean = false, +) { + init { + require(accountId.isNotBlank()) + require(remotePath.isNotBlank() && remotePath.split('/').none { it.isEmpty() || it == "." || it == ".." }) + require(expectedRemoteEtag.isNotBlank() && '\r' !in expectedRemoteEtag && '\n' !in expectedRemoteEtag) + require(staging.isFile && manifest.isFile) + } + + fun markReadyAndActive() = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + val payload = JSONObject(manifest.readText()).put("ready", true).toString().encodeToByteArray() + val temporary = File.createTempFile("manifest-", ".tmp", manifest.parentFile) + try { + FileOutputStream(temporary).use { output -> + output.write(payload) + output.fd.sync() + } + try { + Files.move( + temporary.toPath(), + manifest.toPath(), + StandardCopyOption.ATOMIC_MOVE, + StandardCopyOption.REPLACE_EXISTING, + ) + } catch (_: AtomicMoveNotSupportedException) { + Files.move(temporary.toPath(), manifest.toPath(), StandardCopyOption.REPLACE_EXISTING) + } + ACTIVE_ANDROID_DOCUMENT_WRITEBACKS += manifest.activeWritebackKey() + } finally { + temporary.delete() + } + } + + fun markConflict(observedRemoteEtag: String?) = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + val data = JSONObject(manifest.readText()) + .put("conflict", true) + .put("observedEtag", observedRemoteEtag ?: JSONObject.NULL) + val payload = data.toString().encodeToByteArray() + require(payload.size <= 64 * 1024) + val temporary = File.createTempFile("manifest-", ".tmp", manifest.parentFile) + try { + FileOutputStream(temporary).use { output -> + output.write(payload) + output.fd.sync() + } + try { + Files.move( + temporary.toPath(), + manifest.toPath(), + StandardCopyOption.ATOMIC_MOVE, + StandardCopyOption.REPLACE_EXISTING, + ) + } catch (_: AtomicMoveNotSupportedException) { + Files.move(temporary.toPath(), manifest.toPath(), StandardCopyOption.REPLACE_EXISTING) + } + } finally { + temporary.delete() + } + } + + fun complete() = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + staging.delete() + manifest.delete() + ACTIVE_ANDROID_DOCUMENT_WRITEBACKS -= manifest.activeWritebackKey() + ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= activeWritebackPath() + } + + fun discard() = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + manifest.delete() + staging.delete() + ACTIVE_ANDROID_DOCUMENT_WRITEBACKS -= manifest.activeWritebackKey() + ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= activeWritebackPath() + } + + fun releaseActive() = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + ACTIVE_ANDROID_DOCUMENT_WRITEBACKS -= manifest.activeWritebackKey() + ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= activeWritebackPath() + } + + private fun activeWritebackPath() = ActiveAndroidDocumentWritebackPath(accountId, remotePath) +} + +internal fun androidDocumentPendingWritebackCount(context: android.content.Context, session: NextcloudSession): Int { + return androidDocumentPendingWritebacks(context, session).size +} + +internal fun androidDocumentPendingWritebacks( + context: android.content.Context, + session: NextcloudSession, +): List = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + val root = File(context.filesDir, "documents-recovery") + if (!root.isDirectory) return emptyList() + val accountId = NextcloudDocumentIds.accountKey(session) + return root.listFiles().orEmpty().mapNotNull { manifest -> + parseAndroidDocumentWriteback(root, manifest, accountId) + }.filterNot { writeback -> + writeback.manifest.activeWritebackKey() in ACTIVE_ANDROID_DOCUMENT_WRITEBACKS + }.sortedBy { writeback -> writeback.manifest.lastModified() } +} + +internal fun androidDocumentPendingWriteback( + context: android.content.Context?, + session: NextcloudSession, + remotePath: String, +): AndroidDocumentPendingWriteback? = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + val root = context?.let { File(it.filesDir, "documents-recovery") } ?: return null + if (!root.isDirectory) return null + val account = NextcloudDocumentIds.accountKey(session) + return root.listFiles().orEmpty().asSequence() + .mapNotNull { manifest -> parseAndroidDocumentWriteback(root, manifest, account) } + .filter { writeback -> writeback.remotePath == remotePath } + .filterNot { writeback -> + writeback.manifest.activeWritebackKey() in ACTIVE_ANDROID_DOCUMENT_WRITEBACKS + } + .maxByOrNull { writeback -> writeback.manifest.lastModified() } +} + +internal fun claimAndroidDocumentPendingWriteback( + context: android.content.Context?, + session: NextcloudSession, + remotePath: String, +): AndroidDocumentPendingWriteback? = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + androidDocumentPendingWriteback(context, session, remotePath)?.also { writeback -> + ACTIVE_ANDROID_DOCUMENT_WRITEBACKS += writeback.manifest.activeWritebackKey() + } +} + +internal fun claimAndroidDocumentPendingWritebackForRecovery( + context: android.content.Context, + session: NextcloudSession, + remotePath: String, +): AndroidDocumentPendingWriteback? = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + val activePath = ActiveAndroidDocumentWritebackPath(NextcloudDocumentIds.accountKey(session), remotePath) + if (!ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.add(activePath)) return null + val pending = androidDocumentPendingWriteback(context, session, remotePath) + if (pending == null) { + ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= activePath + return null + } + ACTIVE_ANDROID_DOCUMENT_WRITEBACKS += pending.manifest.activeWritebackKey() + pending +} + +internal fun reserveAndroidDocumentWritebackPath(session: NextcloudSession, remotePath: String) = + synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + val active = ActiveAndroidDocumentWritebackPath(NextcloudDocumentIds.accountKey(session), remotePath) + check(ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.add(active)) { + "This document already has an active local edit." + } + } + +internal fun releaseAndroidDocumentWritebackPath(session: NextcloudSession, remotePath: String) = + synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= + ActiveAndroidDocumentWritebackPath(NextcloudDocumentIds.accountKey(session), remotePath) + } + +internal fun withNoBlockingAndroidDocumentWriteback( + context: android.content.Context?, + session: NextcloudSession, + vararg remotePaths: String, + operation: () -> T, +): T = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + val accountId = NextcloudDocumentIds.accountKey(session) + val activePaths = ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.asSequence() + .filter { active -> active.accountId == accountId } + .map(ActiveAndroidDocumentWritebackPath::remotePath) + val providerContext = requireNotNull(context) { "Provider context is unavailable." } + val retainedPaths = androidDocumentPendingWritebacks(providerContext, session) + .asSequence() + .map(AndroidDocumentPendingWriteback::remotePath) + check(!androidDocumentWritebacksBlockMutation(activePaths, retainedPaths, *remotePaths)) { + "This document cannot be changed while a local edit still needs recovery." + } + operation() +} + +internal fun androidDocumentWritebacksBlockMutation( + activePaths: Sequence, + retainedPaths: Sequence, + vararg mutationPaths: String, +): Boolean = (activePaths + retainedPaths).any { path -> + androidDocumentWritebackPathBlocksMutation(path, *mutationPaths) +} + +internal fun androidDocumentWritebackPathBlocksMutation( + activePath: String, + vararg mutationPaths: String, +): Boolean = mutationPaths.any { path -> activePath == path || activePath.startsWith("$path/") } + +private fun parseAndroidDocumentWriteback( + root: File, + manifest: File, + expectedAccount: String?, +): AndroidDocumentPendingWriteback? = runCatching { + require(manifest.isFile && manifest.name.endsWith(".stage.json") && manifest.length() <= 64 * 1024L) + val data = JSONObject(manifest.readText()) + val stageName = data.getString("stage") + require(data.getInt("version") == 1 && data.optBoolean("ready", false)) + val account = data.getString("account") + require(expectedAccount == null || account == expectedAccount) + require(data.getLong("startedAt") >= 0L) + require(stageName.startsWith("writeback-") && stageName.endsWith(".stage")) + require('/' !in stageName && '\\' !in stageName) + require(manifest.name == "$stageName.json") + val stage = File(root, stageName) + require(stage.isFile) + AndroidDocumentPendingWriteback( + staging = stage, + manifest = manifest, + accountId = account, + remotePath = data.getString("path"), + expectedRemoteEtag = data.getString("etag"), + conflict = data.optBoolean("conflict", false), + ) +}.getOrNull() + +/** Removes writeback transactions that could not reach the close-ready state before process death. */ +internal fun cleanupIncompleteAndroidDocumentWritebacks(context: android.content.Context): Int = + synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + val root = File(context.filesDir, "documents-recovery") + if (!root.isDirectory) return 0 + val files = root.listFiles().orEmpty().filter(File::isFile) + val retainedNames = files.mapNotNull { manifest -> + parseAndroidDocumentWriteback(root, manifest, expectedAccount = null) + }.flatMapTo(hashSetOf()) { writeback -> + listOf(writeback.staging.name, writeback.manifest.name) + } + return files.count { file -> + val owned = + (file.name.startsWith("writeback-") && file.name.endsWith(".stage")) || + (file.name.startsWith("writeback-") && file.name.endsWith(".stage.json")) || + (file.name.startsWith("manifest-") && file.name.endsWith(".tmp")) + owned && file.name !in retainedNames && file.delete() + } + } + +private fun File.activeWritebackKey(): String = absoluteFile.normalize().path + +private data class ActiveAndroidDocumentWritebackPath( + val accountId: String, + val remotePath: String, +) + +private val ANDROID_DOCUMENT_WRITEBACK_LOCK = Any() +private val ACTIVE_ANDROID_DOCUMENT_WRITEBACKS = ConcurrentHashMap.newKeySet() +private val ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS = + ConcurrentHashMap.newKeySet() diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt index a374e21b8..13379d102 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt @@ -32,7 +32,6 @@ import java.nio.file.StandardCopyOption import java.time.ZonedDateTime import java.time.format.DateTimeFormatter import okhttp3.OkHttpClient -import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.atomic.AtomicInteger import org.json.JSONObject import kotlinx.coroutines.Dispatchers @@ -495,7 +494,7 @@ class NextcloudDocumentsProvider : DocumentsProvider() { val destination = childPath(NextcloudDocumentIds.parentPath(reference.path), requireSafeDisplayName(displayName)) if (destination == reference.path) return documentId val etag = requireMutationEtag(file) - withNoActiveAndroidDocumentWriteback(session, reference.path, destination) { + withNoBlockingAndroidDocumentWriteback(context, session, reference.path, destination) { mutationCall { webDav.move(session, account.userId, reference.path, destination, etag) } } notifyMove(session, reference.path, destination) @@ -508,7 +507,7 @@ class NextcloudDocumentsProvider : DocumentsProvider() { if (reference.isRoot) throw SecurityException("The Nextcloud root cannot be deleted.") val account = resolveAccount(session) val file = findDocument(session, account, reference.path) - withNoActiveAndroidDocumentWriteback(session, reference.path) { + withNoBlockingAndroidDocumentWriteback(context, session, reference.path) { mutationCall { webDav.delete( session, @@ -540,7 +539,7 @@ class NextcloudDocumentsProvider : DocumentsProvider() { val file = findDocument(session, account, source.path) val destination = childPath(targetParent.path, file.name) if (destination == source.path) return sourceDocumentId - withNoActiveAndroidDocumentWriteback(session, source.path, destination) { + withNoBlockingAndroidDocumentWriteback(context, session, source.path, destination) { mutationCall { webDav.move(session, account.userId, source.path, destination, requireMutationEtag(file)) } @@ -1002,259 +1001,3 @@ class NextcloudDocumentsProvider : DocumentsProvider() { ) } } - -internal const val MAX_ANDROID_DOCUMENT_WRITEBACK_BYTES = Long.MAX_VALUE -internal const val MIN_ANDROID_DOCUMENT_FREE_BYTES = 512L * 1024L * 1024L - -internal fun requireAndroidDocumentWritebackCapacity(remoteSize: Long, availableBytes: Long) { - require(remoteSize >= 0L && availableBytes >= 0L) - require(remoteSize <= (availableBytes - MIN_ANDROID_DOCUMENT_FREE_BYTES).coerceAtLeast(0L)) { - "There is not enough free space to stage this edit safely." - } -} - -internal fun requireAndroidDocumentStagedWritebackCapacity(stagedBytes: Long, availableBytes: Long) { - require(stagedBytes >= 0L && availableBytes >= 0L) - require(availableBytes >= MIN_ANDROID_DOCUMENT_FREE_BYTES) { - "There is not enough free space to retain this edit safely." - } -} - -internal data class AndroidDocumentPendingWriteback( - val staging: File, - val manifest: File, - val accountId: String, - val remotePath: String, - val expectedRemoteEtag: String, - val conflict: Boolean = false, -) { - init { - require(accountId.isNotBlank()) - require(remotePath.isNotBlank() && remotePath.split('/').none { it.isEmpty() || it == "." || it == ".." }) - require(expectedRemoteEtag.isNotBlank() && '\r' !in expectedRemoteEtag && '\n' !in expectedRemoteEtag) - require(staging.isFile && manifest.isFile) - } - - fun markReadyAndActive() = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val payload = JSONObject(manifest.readText()).put("ready", true).toString().encodeToByteArray() - val temporary = File.createTempFile("manifest-", ".tmp", manifest.parentFile) - try { - FileOutputStream(temporary).use { output -> - output.write(payload) - output.fd.sync() - } - try { - Files.move( - temporary.toPath(), - manifest.toPath(), - StandardCopyOption.ATOMIC_MOVE, - StandardCopyOption.REPLACE_EXISTING, - ) - } catch (_: AtomicMoveNotSupportedException) { - Files.move(temporary.toPath(), manifest.toPath(), StandardCopyOption.REPLACE_EXISTING) - } - ACTIVE_ANDROID_DOCUMENT_WRITEBACKS += manifest.activeWritebackKey() - } finally { - temporary.delete() - } - } - - fun markConflict(observedRemoteEtag: String?) = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val data = JSONObject(manifest.readText()) - .put("conflict", true) - .put("observedEtag", observedRemoteEtag ?: JSONObject.NULL) - val payload = data.toString().encodeToByteArray() - require(payload.size <= 64 * 1024) - val temporary = File.createTempFile("manifest-", ".tmp", manifest.parentFile) - try { - FileOutputStream(temporary).use { output -> - output.write(payload) - output.fd.sync() - } - try { - Files.move( - temporary.toPath(), - manifest.toPath(), - StandardCopyOption.ATOMIC_MOVE, - StandardCopyOption.REPLACE_EXISTING, - ) - } catch (_: AtomicMoveNotSupportedException) { - Files.move(temporary.toPath(), manifest.toPath(), StandardCopyOption.REPLACE_EXISTING) - } - } finally { - temporary.delete() - } - } - - fun complete() = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - staging.delete() - manifest.delete() - ACTIVE_ANDROID_DOCUMENT_WRITEBACKS -= manifest.activeWritebackKey() - ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= activeWritebackPath() - } - - fun discard() = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - manifest.delete() - staging.delete() - ACTIVE_ANDROID_DOCUMENT_WRITEBACKS -= manifest.activeWritebackKey() - ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= activeWritebackPath() - } - - fun releaseActive() = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - ACTIVE_ANDROID_DOCUMENT_WRITEBACKS -= manifest.activeWritebackKey() - ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= activeWritebackPath() - } - - private fun activeWritebackPath() = ActiveAndroidDocumentWritebackPath(accountId, remotePath) -} - -internal fun androidDocumentPendingWritebackCount(context: android.content.Context, session: NextcloudSession): Int { - return androidDocumentPendingWritebacks(context, session).size -} - -internal fun androidDocumentPendingWritebacks( - context: android.content.Context, - session: NextcloudSession, -): List = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val root = File(context.filesDir, "documents-recovery") - if (!root.isDirectory) return emptyList() - val account = NextcloudDocumentIds.accountKey(session) - return root.listFiles().orEmpty().mapNotNull { manifest -> - parseAndroidDocumentWriteback(root, manifest, account) - }.filterNot { writeback -> - writeback.manifest.activeWritebackKey() in ACTIVE_ANDROID_DOCUMENT_WRITEBACKS - }.sortedBy { writeback -> writeback.manifest.lastModified() } -} - -internal fun androidDocumentPendingWriteback( - context: android.content.Context?, - session: NextcloudSession, - remotePath: String, -): AndroidDocumentPendingWriteback? = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val root = context?.let { File(it.filesDir, "documents-recovery") } ?: return null - if (!root.isDirectory) return null - val account = NextcloudDocumentIds.accountKey(session) - return root.listFiles().orEmpty().asSequence() - .mapNotNull { manifest -> parseAndroidDocumentWriteback(root, manifest, account) } - .filter { writeback -> writeback.remotePath == remotePath } - .filterNot { writeback -> - writeback.manifest.activeWritebackKey() in ACTIVE_ANDROID_DOCUMENT_WRITEBACKS - } - .maxByOrNull { writeback -> writeback.manifest.lastModified() } -} - -private fun claimAndroidDocumentPendingWriteback( - context: android.content.Context?, - session: NextcloudSession, - remotePath: String, -): AndroidDocumentPendingWriteback? = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - androidDocumentPendingWriteback(context, session, remotePath)?.also { writeback -> - ACTIVE_ANDROID_DOCUMENT_WRITEBACKS += writeback.manifest.activeWritebackKey() - } -} - -internal fun claimAndroidDocumentPendingWritebackForRecovery( - context: android.content.Context, - session: NextcloudSession, - remotePath: String, -): AndroidDocumentPendingWriteback? = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val activePath = ActiveAndroidDocumentWritebackPath(NextcloudDocumentIds.accountKey(session), remotePath) - if (!ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.add(activePath)) return null - val pending = androidDocumentPendingWriteback(context, session, remotePath) - if (pending == null) { - ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= activePath - return null - } - ACTIVE_ANDROID_DOCUMENT_WRITEBACKS += pending.manifest.activeWritebackKey() - pending -} - -private fun reserveAndroidDocumentWritebackPath(session: NextcloudSession, remotePath: String) = - synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val active = ActiveAndroidDocumentWritebackPath(NextcloudDocumentIds.accountKey(session), remotePath) - check(ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.add(active)) { - "This document already has an active local edit." - } - } - -private fun releaseAndroidDocumentWritebackPath(session: NextcloudSession, remotePath: String) = - synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS -= - ActiveAndroidDocumentWritebackPath(NextcloudDocumentIds.accountKey(session), remotePath) - } - -private fun withNoActiveAndroidDocumentWriteback( - session: NextcloudSession, - vararg remotePaths: String, - operation: () -> T, -): T = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val accountId = NextcloudDocumentIds.accountKey(session) - check(ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.none { active -> - active.accountId == accountId && androidDocumentWritebackPathBlocksMutation(active.remotePath, *remotePaths) - }) { "This document cannot be changed while a local edit is still open." } - operation() -} - -internal fun androidDocumentWritebackPathBlocksMutation( - activePath: String, - vararg mutationPaths: String, -): Boolean = mutationPaths.any { path -> activePath == path || activePath.startsWith("$path/") } - -private fun parseAndroidDocumentWriteback( - root: File, - manifest: File, - expectedAccount: String?, -): AndroidDocumentPendingWriteback? = runCatching { - require(manifest.isFile && manifest.name.endsWith(".stage.json") && manifest.length() <= 64 * 1024L) - val data = JSONObject(manifest.readText()) - val stageName = data.getString("stage") - require(data.getInt("version") == 1 && data.optBoolean("ready", false)) - val account = data.getString("account") - require(expectedAccount == null || account == expectedAccount) - require(data.getLong("startedAt") >= 0L) - require(stageName.startsWith("writeback-") && stageName.endsWith(".stage")) - require('/' !in stageName && '\\' !in stageName) - require(manifest.name == "$stageName.json") - val stage = File(root, stageName) - require(stage.isFile) - AndroidDocumentPendingWriteback( - staging = stage, - manifest = manifest, - accountId = account, - remotePath = data.getString("path"), - expectedRemoteEtag = data.getString("etag"), - conflict = data.optBoolean("conflict", false), - ) -}.getOrNull() - -/** Removes writeback transactions that could not reach the close-ready state before process death. */ -internal fun cleanupIncompleteAndroidDocumentWritebacks(context: android.content.Context): Int = - synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val root = File(context.filesDir, "documents-recovery") - if (!root.isDirectory) return 0 - val files = root.listFiles().orEmpty().filter(File::isFile) - val retainedNames = files.mapNotNull { manifest -> - parseAndroidDocumentWriteback(root, manifest, expectedAccount = null) - }.flatMapTo(hashSetOf()) { writeback -> - listOf(writeback.staging.name, writeback.manifest.name) - } - return files.count { file -> - val owned = - (file.name.startsWith("writeback-") && file.name.endsWith(".stage")) || - (file.name.startsWith("writeback-") && file.name.endsWith(".stage.json")) || - (file.name.startsWith("manifest-") && file.name.endsWith(".tmp")) - owned && file.name !in retainedNames && file.delete() - } - } - -private fun File.activeWritebackKey(): String = absoluteFile.normalize().path - -private data class ActiveAndroidDocumentWritebackPath( - val accountId: String, - val remotePath: String, -) - -private val ANDROID_DOCUMENT_WRITEBACK_LOCK = Any() -private val ACTIVE_ANDROID_DOCUMENT_WRITEBACKS = ConcurrentHashMap.newKeySet() -private val ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS = - ConcurrentHashMap.newKeySet() diff --git a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt index e3e534274..af7cf2efa 100644 --- a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt +++ b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt @@ -55,6 +55,31 @@ class AndroidVirtualFileProxyCallbackTest { ) } + @Test + fun `process restored writeback blocks destructive mutations until recovery`() { + assertTrue( + androidDocumentWritebacksBlockMutation( + emptySequence(), + sequenceOf("Projects/Active/notes.txt"), + "Projects/Active", + ), + ) + assertTrue( + androidDocumentWritebacksBlockMutation( + emptySequence(), + sequenceOf("Projects/Active/notes.txt"), + "Projects/Active/notes.txt", + ), + ) + assertFalse( + androidDocumentWritebacksBlockMutation( + emptySequence(), + sequenceOf("Projects/Active/notes.txt"), + "Projects/Archive", + ), + ) + } + @Test fun `writable proxy capacity preserves the free space reserve`() { assertTrue(androidDocumentWriteFitsCapacity(40L, 50L, 110L, reserveBytes = 100L)) From 063141e1c4038258d79defa2b0df7bed104a06c8 Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Tue, 1 Sep 2026 21:40:45 +0200 Subject: [PATCH 2/8] chore(changelog): link pull request --- changes/unreleased/android-writeback-mutation-guard.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changes/unreleased/android-writeback-mutation-guard.md diff --git a/changes/unreleased/android-writeback-mutation-guard.md b/changes/unreleased/android-writeback-mutation-guard.md new file mode 100644 index 000000000..c9af01a8d --- /dev/null +++ b/changes/unreleased/android-writeback-mutation-guard.md @@ -0,0 +1,7 @@ +category: fix +issue: none +pull: 435 +platforms: android +user-facing: yes + +Android document rename, move, and delete actions now preserve edits retained for restart recovery instead of stranding them at an obsolete remote path. From 834497f0764d0a53a546c72f7cff71b109483492 Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Tue, 1 Sep 2026 21:53:40 +0200 Subject: [PATCH 3/8] chore(architecture): tighten Android provider size baseline --- tools/kotlin-file-size-baseline.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/kotlin-file-size-baseline.txt b/tools/kotlin-file-size-baseline.txt index bce4b798b..3bdd2977f 100644 --- a/tools/kotlin-file-size-baseline.txt +++ b/tools/kotlin-file-size-baseline.txt @@ -1,7 +1,7 @@ androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncEngine.kt|851 androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt|4269 androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidProjectContentClient.kt|985 -androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt|1282 +androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt|1003 contractAcquisition/src/main/kotlin/dev/obiente/nextcloudnative/contracts/SignedAppStoreContractAcquirer.kt|1224 contractAcquisition/src/main/kotlin/dev/obiente/nextcloudnative/contracts/StaticRouteContract.kt|1883 contractAcquisition/src/test/kotlin/dev/obiente/nextcloudnative/contracts/SignedAppStoreContractAcquirerTest.kt|1798 From 6b172c229ac295f7940098cde91022cadc14931c Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Fri, 4 Sep 2026 02:38:17 +0200 Subject: [PATCH 4/8] fix(android): preserve ambiguous writeback recovery --- .../AndroidDocumentWritebackRecovery.kt | 85 ++++++++++++++++--- .../AndroidNextcloudServices.kt | 6 +- .../AndroidVirtualFileProxyCallbackTest.kt | 41 +++++++++ 3 files changed, 117 insertions(+), 15 deletions(-) diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt index 05e8e6d07..41b9e29d1 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt @@ -7,6 +7,7 @@ import java.nio.file.AtomicMoveNotSupportedException import java.nio.file.Files import java.nio.file.StandardCopyOption import java.util.concurrent.ConcurrentHashMap +import kotlinx.coroutines.CancellationException import org.json.JSONObject internal const val MAX_ANDROID_DOCUMENT_WRITEBACK_BYTES = Long.MAX_VALUE @@ -124,8 +125,9 @@ internal fun androidDocumentPendingWritebacks( ): List = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { val root = File(context.filesDir, "documents-recovery") if (!root.isDirectory) return emptyList() + val files = requireInspectableAndroidDocumentWritebackRecovery(root) val accountId = NextcloudDocumentIds.accountKey(session) - return root.listFiles().orEmpty().mapNotNull { manifest -> + return files.mapNotNull { manifest -> parseAndroidDocumentWriteback(root, manifest, accountId) }.filterNot { writeback -> writeback.manifest.activeWritebackKey() in ACTIVE_ANDROID_DOCUMENT_WRITEBACKS @@ -139,8 +141,9 @@ internal fun androidDocumentPendingWriteback( ): AndroidDocumentPendingWriteback? = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { val root = context?.let { File(it.filesDir, "documents-recovery") } ?: return null if (!root.isDirectory) return null + val files = requireInspectableAndroidDocumentWritebackRecovery(root) val account = NextcloudDocumentIds.accountKey(session) - return root.listFiles().orEmpty().asSequence() + return files.asSequence() .mapNotNull { manifest -> parseAndroidDocumentWriteback(root, manifest, account) } .filter { writeback -> writeback.remotePath == remotePath } .filterNot { writeback -> @@ -222,15 +225,28 @@ internal fun androidDocumentWritebackPathBlocksMutation( vararg mutationPaths: String, ): Boolean = mutationPaths.any { path -> activePath == path || activePath.startsWith("$path/") } -private fun parseAndroidDocumentWriteback( +internal inline fun handleAndroidDocumentWritebackRecoveryFailure( + failure: Throwable, + release: () -> Unit, +) { + release() + if (failure is CancellationException) throw failure +} + +private data class AndroidDocumentWritebackManifest( + val pending: AndroidDocumentPendingWriteback, + val ready: Boolean, +) + +private fun parseAndroidDocumentWritebackManifest( root: File, manifest: File, expectedAccount: String?, -): AndroidDocumentPendingWriteback? = runCatching { +): AndroidDocumentWritebackManifest? = runCatching { require(manifest.isFile && manifest.name.endsWith(".stage.json") && manifest.length() <= 64 * 1024L) val data = JSONObject(manifest.readText()) val stageName = data.getString("stage") - require(data.getInt("version") == 1 && data.optBoolean("ready", false)) + require(data.getInt("version") == 1) val account = data.getString("account") require(expectedAccount == null || account == expectedAccount) require(data.getLong("startedAt") >= 0L) @@ -239,27 +255,68 @@ private fun parseAndroidDocumentWriteback( require(manifest.name == "$stageName.json") val stage = File(root, stageName) require(stage.isFile) - AndroidDocumentPendingWriteback( - staging = stage, - manifest = manifest, - accountId = account, - remotePath = data.getString("path"), - expectedRemoteEtag = data.getString("etag"), - conflict = data.optBoolean("conflict", false), + AndroidDocumentWritebackManifest( + pending = AndroidDocumentPendingWriteback( + staging = stage, + manifest = manifest, + accountId = account, + remotePath = data.getString("path"), + expectedRemoteEtag = data.getString("etag"), + conflict = data.optBoolean("conflict", false), + ), + ready = data.optBoolean("ready", false), ) }.getOrNull() +private fun parseAndroidDocumentWriteback( + root: File, + manifest: File, + expectedAccount: String?, +): AndroidDocumentPendingWriteback? = parseAndroidDocumentWritebackManifest(root, manifest, expectedAccount) + ?.takeIf(AndroidDocumentWritebackManifest::ready) + ?.pending + +internal fun requireInspectableAndroidDocumentWritebackRecovery(root: File): List { + if (!root.exists()) return emptyList() + check(root.isDirectory) { "Document writeback recovery storage is not a directory." } + val files = requireNotNull(root.listFiles()) { "Document writeback recovery storage could not be inspected." } + .filter(File::isFile) + val ambiguousManifest = files.firstOrNull { manifest -> + manifest.name.startsWith("writeback-") && + manifest.name.endsWith(".stage.json") && + File(root, manifest.name.removeSuffix(".json")).isFile && + parseAndroidDocumentWritebackManifest(root, manifest, expectedAccount = null) == null + } + check(ambiguousManifest == null) { + "A retained document edit has recovery metadata that cannot be inspected safely." + } + return files +} + /** Removes writeback transactions that could not reach the close-ready state before process death. */ internal fun cleanupIncompleteAndroidDocumentWritebacks(context: android.content.Context): Int = + cleanupIncompleteAndroidDocumentWritebacks(File(context.filesDir, "documents-recovery")) + +internal fun cleanupIncompleteAndroidDocumentWritebacks(root: File): Int = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val root = File(context.filesDir, "documents-recovery") if (!root.isDirectory) return 0 - val files = root.listFiles().orEmpty().filter(File::isFile) + val files = requireNotNull(root.listFiles()) { + "Document writeback recovery storage could not be inspected." + }.filter(File::isFile) val retainedNames = files.mapNotNull { manifest -> parseAndroidDocumentWriteback(root, manifest, expectedAccount = null) }.flatMapTo(hashSetOf()) { writeback -> listOf(writeback.staging.name, writeback.manifest.name) } + files.filter { manifest -> + manifest.name.startsWith("writeback-") && + manifest.name.endsWith(".stage.json") && + File(root, manifest.name.removeSuffix(".json")).isFile && + parseAndroidDocumentWritebackManifest(root, manifest, expectedAccount = null) == null + }.forEach { manifest -> + retainedNames += manifest.name + retainedNames += manifest.name.removeSuffix(".json") + } return files.count { file -> val owned = (file.name.startsWith("writeback-") && file.name.endsWith(".stage")) || diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt index 402a975b9..68e6a0620 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt @@ -1598,6 +1598,7 @@ internal class AndroidNextcloudServices( cloudMutationsAllowed = appContext.cloudMutationGate(), ) documentWritebacks.forEach { discovered -> + currentCoroutineContext().ensureActive() val pending = claimAndroidDocumentPendingWritebackForRecovery( appContext, session, @@ -1618,6 +1619,7 @@ internal class AndroidNextcloudServices( userId = userId, pending = pending, ) + currentCoroutineContext().ensureActive() if (remote.contentsMatch) { virtualFileCache.invalidate(session, pending.remotePath) notifyDocumentsDocumentChanged(session, pending.remotePath) @@ -1639,7 +1641,9 @@ internal class AndroidNextcloudServices( virtualFileCache.invalidate(session, pending.remotePath) notifyDocumentsDocumentChanged(session, pending.remotePath) pending.complete() - }.onFailure { pending.releaseActive() } + }.onFailure { failure -> + handleAndroidDocumentWritebackRecoveryFailure(failure, pending::releaseActive) + } } } val pendingWritebacks = androidDocumentPendingWritebackCount(appContext, session) diff --git a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt index af7cf2efa..285b5e0df 100644 --- a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt +++ b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt @@ -8,9 +8,50 @@ import kotlin.test.assertContentEquals import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertFailsWith +import kotlin.test.assertSame import kotlin.test.assertTrue +import kotlinx.coroutines.CancellationException class AndroidVirtualFileProxyCallbackTest { + @Test + fun `ambiguous writeback metadata preserves retained user bytes and blocks discovery`() { + val root = Files.createTempDirectory("document-writeback-recovery-").toFile() + val staging = root.resolve("writeback-retained.stage").apply { + writeText("retained user edit") + } + val manifest = root.resolve("${staging.name}.json").apply { + writeText("{\"version\":2}") + } + + try { + assertEquals(0, cleanupIncompleteAndroidDocumentWritebacks(root)) + assertTrue(staging.isFile) + assertEquals("retained user edit", staging.readText()) + assertTrue(manifest.isFile) + + assertFailsWith { + requireInspectableAndroidDocumentWritebackRecovery(root) + } + } finally { + root.deleteRecursively() + } + } + + @Test + fun `writeback recovery releases ownership and rethrows cancellation`() { + val cancellation = CancellationException("recovery cancelled") + var releases = 0 + + val thrown = assertFailsWith { + handleAndroidDocumentWritebackRecoveryFailure(cancellation) { + releases += 1 + } + } + + assertSame(cancellation, thrown) + assertEquals(1, releases) + } + @Test fun `writeback reconciliation compares remote bytes without replacing the retained stage`() { val staging = Files.createTempFile("writeback-compare-", ".stage").toFile().apply { From a2d4b7c8f88d0dc8b59aee3e680cbf70f06ee36c Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Fri, 4 Sep 2026 03:16:23 +0200 Subject: [PATCH 5/8] fix(android): guard retained document recovery --- ...AndroidVirtualFileCacheInstrumentedTest.kt | 53 ++++++- .../nextcloudnative/AndroidFileSyncEngine.kt | 4 +- .../AndroidFileSyncRemoteOwnership.kt | 3 + .../AndroidFileSyncRemoteTree.kt | 139 +++++++++++------- .../AndroidNextcloudServices.kt | 102 +++++++------ .../NextcloudDocumentWebDav.kt | 12 +- ...cumentAtomicReplacementCancellationTest.kt | 134 +++++++++++++++++ 7 files changed, 344 insertions(+), 103 deletions(-) create mode 100644 androidApp/src/test/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentAtomicReplacementCancellationTest.kt diff --git a/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt index 92cb4bc46..5bc789d72 100644 --- a/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt +++ b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt @@ -7,6 +7,8 @@ import dev.obiente.nextcloudnative.app.NextcloudFile import dev.obiente.nextcloudnative.app.NextcloudSession import dev.obiente.nextcloudnative.app.VirtualFileCachePolicy import java.io.File +import mockwebserver3.MockResponse +import mockwebserver3.MockWebServer import org.junit.After import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals @@ -145,11 +147,13 @@ class AndroidVirtualFileCacheInstrumentedTest { .toString(), ) + var blockedMutationRan = false org.junit.Assert.assertThrows(IllegalStateException::class.java) { withNoBlockingAndroidDocumentWriteback(context, session, "Projects/Active") { - error("The blocked mutation must not run.") + blockedMutationRan = true } } + org.junit.Assert.assertFalse(blockedMutationRan) var unrelatedMutationRan = false withNoBlockingAndroidDocumentWriteback(context, session, "Projects/Archive") { unrelatedMutationRan = true @@ -157,6 +161,53 @@ class AndroidVirtualFileCacheInstrumentedTest { org.junit.Assert.assertTrue(unrelatedMutationRan) } + @Test + fun processRestoredWritebackBlocksFileSyncRemoteDeletion() { + MockWebServer().use { server -> + server.start() + val syncSession = session.copy(serverUrl = server.url("/").toString()) + val recovery = File(context.filesDir, "documents-recovery").apply { mkdirs() } + val stage = File(recovery, "writeback-sync-guard.stage").apply { writeText("local edit") } + File(recovery, stage.name + ".json").writeText( + JSONObject() + .put("version", 1) + .put("account", NextcloudDocumentIds.accountKey(syncSession)) + .put("path", "Projects/Active/notes.txt") + .put("etag", "\"v1\"") + .put("displayName", "notes.txt") + .put("stage", stage.name) + .put("startedAt", 10L) + .put("ready", true) + .toString(), + ) + server.enqueue( + MockResponse.Builder().code(207).body( + """ + + /remote.php/dav/files/virtual-cache-fixture/Projects/Active/notes.txt + notes.txt + "v1"10 + + + + """.trimIndent(), + ).build(), + ) + val remote = AndroidFileSyncRemoteTree( + session = syncSession, + userId = "virtual-cache-fixture", + remoteRootPath = "", + webDav = NextcloudDocumentWebDav(), + documentWritebackContext = context, + ) + + org.junit.Assert.assertThrows(IllegalStateException::class.java) { + remote.delete("Projects/Active/notes.txt", "\"v1\"") + } + assertEquals(1, server.requestCount) + } + } + @Test fun providerStartupDiscardsIncompleteWritebacksAndKeepsReadyRecovery() { val recovery = File(context.filesDir, "documents-recovery").apply { mkdirs() } diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncEngine.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncEngine.kt index b6562eec5..d24013c6e 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncEngine.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncEngine.kt @@ -303,7 +303,7 @@ internal class AndroidFileSyncEngine(context: Context) { ) } val ownedUploads = fileSyncOwnedUploads(pair) - val remote = androidFileSyncOwnedRemoteTree(session, userId, pair, webDav) + val remote = androidFileSyncOwnedRemoteTree(session, userId, pair, webDav, context = appContext) val cleanupResult = cleanupJvmFileSyncOwnedUploads( remote, current.coordinator, pairId, ownedUploads, ) @@ -401,7 +401,7 @@ internal class AndroidFileSyncEngine(context: Context) { return withAndroidMediaBackupLedger(appContext, initialPair) { mediaLedger -> val remote = androidFileSyncOwnedRemoteTree( session, userId, initialPair, webDav, - transferCancellation = transferCancellation, + transferCancellation = transferCancellation, context = appContext, ) val cleanupResult = cleanupJvmFileSyncOwnedUploads( remote, persisted.coordinator, pairId, initialPair.pendingUploadCleanups, diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncRemoteOwnership.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncRemoteOwnership.kt index 61f4141b1..6d5ea36bd 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncRemoteOwnership.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncRemoteOwnership.kt @@ -1,5 +1,6 @@ package dev.obiente.nextcloudnative +import android.content.Context import dev.obiente.nextcloudnative.app.FileSyncPair import dev.obiente.nextcloudnative.app.NextcloudSession import dev.obiente.nextcloudnative.app.fileSyncOwnedReplacementBackupEtags @@ -15,6 +16,7 @@ internal fun androidFileSyncOwnedRemoteTree( transferCancellation: DocumentRequestCancellation = AndroidFileSyncRunCancellation { !Thread.currentThread().isInterrupted }, + context: Context? = null, ): AndroidFileSyncRemoteTree { val ownedUploads = fileSyncOwnedUploads(pair) return AndroidFileSyncRemoteTree( @@ -27,5 +29,6 @@ internal fun androidFileSyncOwnedRemoteTree( ownedStageEtags = fileSyncOwnedUploadStageEtags(pair), ownedUploadPaths = fileSyncOwnedUploadPaths(pair), ownedReplacementBackupEtags = fileSyncOwnedReplacementBackupEtags(pair), + documentWritebackContext = context?.applicationContext, ) } diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncRemoteTree.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncRemoteTree.kt index fa1917f0f..2fd159760 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncRemoteTree.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidFileSyncRemoteTree.kt @@ -1,5 +1,6 @@ package dev.obiente.nextcloudnative +import android.content.Context import dev.obiente.nextcloudnative.app.NextcloudSession import dev.obiente.nextcloudnative.app.JvmResumableNextcloudUploadRemote import dev.obiente.nextcloudnative.app.JvmExactFileComparisonOutputStream @@ -34,6 +35,7 @@ internal class AndroidFileSyncRemoteTree( private val ownedStageEtags: Map = emptyMap(), private val ownedUploadPaths: Map = emptyMap(), private val ownedReplacementBackupEtags: Map = emptyMap(), + private val documentWritebackContext: Context? = null, ) : JvmResumableNextcloudUploadRemote { private val rootPath = remoteRootPath.trim('/') private val ownedDestinationPaths = ownedUploadPaths.mapValues { (_, path) -> fullPath(path) } @@ -158,7 +160,10 @@ internal class AndroidFileSyncRemoteTree( val current = resolve(relativePath) if (expectedRemoteEtag == null) { require(current == null) { "The server folder appeared after the sync scan." } - webDav.createFolder(session, userId, fullPath(relativePath)) + val remotePath = fullPath(relativePath) + withDocumentWritebackMutationGuard(remotePath) { + webDav.createFolder(session, userId, remotePath) + } } else { require(current?.entry?.etag == expectedRemoteEtag) { "The server folder changed after the sync scan." @@ -169,25 +174,28 @@ internal class AndroidFileSyncRemoteTree( fun writeFile(relativePath: String, source: File, expectedRemoteEtag: String?): RemoteSyncEntry { val current = resolve(relativePath) - if (expectedRemoteEtag == null) { - require(current == null) { "The server file appeared after the sync scan." } - webDav.createFile( - session, userId, fullPath(relativePath), source, - cancellation = transferCancellation, - ) - } else { - require(current?.entry?.etag == expectedRemoteEtag) { - "The server file changed after the sync scan." + val remotePath = fullPath(relativePath) + withDocumentWritebackMutationGuard(remotePath) { + if (expectedRemoteEtag == null) { + require(current == null) { "The server file appeared after the sync scan." } + webDav.createFile( + session, userId, remotePath, source, + cancellation = transferCancellation, + ) + } else { + require(current?.entry?.etag == expectedRemoteEtag) { + "The server file changed after the sync scan." + } + require(!current.isDirectory) { "The server item changed type." } + webDav.replaceFile( + session, + userId, + remotePath, + source, + expectedRemoteEtag, + transferCancellation, + ) } - require(!current.isDirectory) { "The server item changed type." } - webDav.replaceFile( - session, - userId, - fullPath(relativePath), - source, - expectedRemoteEtag, - transferCancellation, - ) } val after = requireNotNull(resolve(relativePath)) { "The uploaded server file disappeared." } require(!after.isDirectory) { "The uploaded server item is not a file." } @@ -328,15 +336,19 @@ internal class AndroidFileSyncRemoteTree( require(resolveIncludingOwnedStage(backupPath) == null) { "The replacement backup already exists." } try { moveReplacementDirectory(fullPath(relativePath), fullPath(backupPath), expectedDirectoryEtag) - webDav.publishChunkUploadStage( - session, - userId, - fullPath(jvmOwnedUploadStagePath(relativePath, uploadId)), - fullPath(relativePath), - verifiedStageEtag, - expectedRemoteEtag = null, - cancellation = transferCancellation, - ) + val fullStagePath = fullPath(jvmOwnedUploadStagePath(relativePath, uploadId)) + val destinationPath = fullPath(relativePath) + withDocumentWritebackMutationGuard(fullStagePath, destinationPath) { + webDav.publishChunkUploadStage( + session, + userId, + fullStagePath, + destinationPath, + verifiedStageEtag, + expectedRemoteEtag = null, + cancellation = transferCancellation, + ) + } val published = requireNotNull(resolveIncludingOwnedStage(relativePath)) { "The uploaded server file disappeared." } @@ -374,15 +386,19 @@ internal class AndroidFileSyncRemoteTree( expectedRemoteEtag: String?, ): RemoteSyncEntry { val stagePath = jvmOwnedUploadStagePath(relativePath, uploadId) - webDav.publishChunkUploadStage( - session, - userId, - fullPath(stagePath), - fullPath(relativePath), - verifiedStageEtag, - expectedRemoteEtag, - transferCancellation, - ) + val fullStagePath = fullPath(stagePath) + val destinationPath = fullPath(relativePath) + withDocumentWritebackMutationGuard(fullStagePath, destinationPath) { + webDav.publishChunkUploadStage( + session, + userId, + fullStagePath, + destinationPath, + verifiedStageEtag, + expectedRemoteEtag, + transferCancellation, + ) + } val after = requireNotNull(resolve(relativePath)) { "The uploaded server file disappeared." } require(!after.isDirectory) return after.entry @@ -587,14 +603,17 @@ internal class AndroidFileSyncRemoteTree( require(current.entry.etag == expectedRemoteEtag) { "The server item changed after the sync scan." } - webDav.delete( - session, - userId, - fullPath(relativePath), - expectedRemoteEtag, - isDirectory = current.isDirectory, - cancellation = transferCancellation, - ) + val remotePath = fullPath(relativePath) + withDocumentWritebackMutationGuard(remotePath) { + webDav.delete( + session, + userId, + remotePath, + expectedRemoteEtag, + isDirectory = current.isDirectory, + cancellation = transferCancellation, + ) + } } private fun listSyncDirectory(relativeParent: String): DocumentDirectoryResult = @@ -715,14 +734,17 @@ internal class AndroidFileSyncRemoteTree( return true } if (!destination.isDirectory && assembledStageEtag != null && destination.entry.etag == assembledStageEtag) { - webDav.delete( - session, - userId, - fullPath(relativePath), - destination.entry.etag, - isDirectory = false, - cancellation = transferCancellation, - ) + val destinationPath = fullPath(relativePath) + withDocumentWritebackMutationGuard(destinationPath) { + webDav.delete( + session, + userId, + destinationPath, + destination.entry.etag, + isDirectory = false, + cancellation = transferCancellation, + ) + } moveReplacementDirectory(fullPath(backupPath), fullPath(relativePath), expectedBackupEtag) return true } @@ -733,7 +755,16 @@ internal class AndroidFileSyncRemoteTree( } private fun moveReplacementDirectory(sourcePath: String, destinationPath: String, expectedEtag: String) = - webDav.moveDirectory(session, userId, sourcePath, destinationPath, expectedEtag, transferCancellation) + withDocumentWritebackMutationGuard(sourcePath, destinationPath) { + webDav.moveDirectory(session, userId, sourcePath, destinationPath, expectedEtag, transferCancellation) + } + + private fun withDocumentWritebackMutationGuard( + vararg remotePaths: String, + operation: () -> T, + ): T = documentWritebackContext?.let { context -> + withNoBlockingAndroidDocumentWriteback(context, session, *remotePaths, operation = operation) + } ?: operation() internal fun fullPath(relativePath: String): String = listOf(rootPath, relativePath.trim('/')).filter(String::isNotBlank).joinToString("/") diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt index 68e6a0620..a8087d2fe 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt @@ -1613,31 +1613,37 @@ internal class AndroidNextcloudServices( stagedBytes = pending.staging.length(), availableBytes = pending.staging.parentFile?.usableSpace ?: 0L, ) - val remote = compareAndroidDocumentWriteback( - webDav = webDav, - session = session, - userId = userId, - pending = pending, - ) - currentCoroutineContext().ensureActive() - if (remote.contentsMatch) { - virtualFileCache.invalidate(session, pending.remotePath) - notifyDocumentsDocumentChanged(session, pending.remotePath) - pending.complete() - return@runCatching - } - if (remote.etag == null || remote.etag != pending.expectedRemoteEtag) { - pending.markConflict(remote.etag) - pending.releaseActive() - return@runCatching + CoroutineDocumentRequestCancellation( + requireNotNull(currentCoroutineContext()[Job]), + ).use { cancellation -> + val remote = compareAndroidDocumentWriteback( + webDav = webDav, + session = session, + userId = userId, + pending = pending, + cancellation = cancellation, + ) + currentCoroutineContext().ensureActive() + if (remote.contentsMatch) { + virtualFileCache.invalidate(session, pending.remotePath) + notifyDocumentsDocumentChanged(session, pending.remotePath) + pending.complete() + return@runCatching + } + if (remote.etag == null || remote.etag != pending.expectedRemoteEtag) { + pending.markConflict(remote.etag) + pending.releaseActive() + return@runCatching + } + webDav.replaceFileAtomically( + session = session, + userId = userId, + path = pending.remotePath, + source = pending.staging, + expectedEtag = pending.expectedRemoteEtag, + cancellation = cancellation, + ) } - webDav.replaceFileAtomically( - session = session, - userId = userId, - path = pending.remotePath, - source = pending.staging, - expectedEtag = pending.expectedRemoteEtag, - ) virtualFileCache.invalidate(session, pending.remotePath) notifyDocumentsDocumentChanged(session, pending.remotePath) pending.complete() @@ -2732,28 +2738,34 @@ internal class AndroidNextcloudServices( mutation: NextcloudFileMutation, ): NextcloudFileMutationResult = withContext(Dispatchers.IO) { val spec = mutation.toWebDavMutationSpec() - val headers = buildMap { - put("Accept", "*/*") - putAll(spec.conflictConditionHeaders()) - spec.destinationPath?.let { destinationPath -> - put("Destination", buildNextcloudFileUrl(session.serverUrl, userId, destinationPath)) - put("Overwrite", if (spec.overwrite) "T" else "F") + withNoBlockingAndroidDocumentWriteback( + appContext, + session, + *listOfNotNull(spec.sourcePath, spec.destinationPath).toTypedArray(), + ) { + val headers = buildMap { + put("Accept", "*/*") + putAll(spec.conflictConditionHeaders()) + spec.destinationPath?.let { destinationPath -> + put("Destination", buildNextcloudFileUrl(session.serverUrl, userId, destinationPath)) + put("Overwrite", if (spec.overwrite) "T" else "F") + } } + val response = request( + method = spec.method, + url = buildNextcloudFileUrl(session.serverUrl, userId, spec.sourcePath), + session = session, + headers = headers, + maxResponseBytes = 64 * 1024, + ) + if (response.status !in 200..299) throw fileOperationException(response.status) + val accountId = NextcloudDocumentIds.accountKey(session) + runCatching { fileReadCache.invalidate(accountId, spec.sourcePath) } + spec.destinationPath?.let { destination -> + runCatching { fileReadCache.invalidate(accountId, destination) } + } + NextcloudFileMutationResult(spec.destinationPath, response.etag) } - val response = request( - method = spec.method, - url = buildNextcloudFileUrl(session.serverUrl, userId, spec.sourcePath), - session = session, - headers = headers, - maxResponseBytes = 64 * 1024, - ) - if (response.status !in 200..299) throw fileOperationException(response.status) - val accountId = NextcloudDocumentIds.accountKey(session) - runCatching { fileReadCache.invalidate(accountId, spec.sourcePath) } - spec.destinationPath?.let { destination -> - runCatching { fileReadCache.invalidate(accountId, destination) } - } - NextcloudFileMutationResult(spec.destinationPath, response.etag) } override suspend fun executeNextcloudApi( @@ -4125,6 +4137,7 @@ private fun compareAndroidDocumentWriteback( session: NextcloudSession, userId: String, pending: AndroidDocumentPendingWriteback, + cancellation: DocumentRequestCancellation, ): AndroidDocumentRemoteComparison = AndroidDocumentStagingComparator(pending.staging).use { comparison -> val result = webDav.readFile( session = session, @@ -4132,6 +4145,7 @@ private fun compareAndroidDocumentWriteback( path = pending.remotePath, destination = comparison, maximumBytes = MAX_ANDROID_DOCUMENT_WRITEBACK_BYTES, + cancellation = cancellation, ) AndroidDocumentRemoteComparison( contentsMatch = comparison.matches(result.byteCount), diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentWebDav.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentWebDav.kt index 2e5ec03f4..22a3458a3 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentWebDav.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentWebDav.kt @@ -346,6 +346,7 @@ internal class NextcloudDocumentWebDav( path: String, source: File, expectedEtag: String, + cancellation: DocumentRequestCancellation = NoDocumentRequestCancellation, ): DocumentMutationResult { require(expectedEtag.isNotBlank()) { "An ETag is required for conflict-protected replacement." } val parent = NextcloudDocumentIds.parentPath(path) @@ -353,9 +354,15 @@ internal class NextcloudDocumentWebDav( val stagingPath = if (parent.isBlank()) stagingName else "$parent/$stagingName" val stagingUrl = buildNextcloudFileUrl(session.serverUrl, userId, stagingPath) val destinationUrl = buildNextcloudFileUrl(session.serverUrl, userId, path) - val staged = createFile(session, userId, stagingPath, source) - val stagedEtag = staged.etag + var stagedEtag: String? = null try { + stagedEtag = createFile( + session = session, + userId = userId, + path = stagingPath, + source = source, + cancellation = cancellation, + ).etag val builder = requestBuilder(session, stagingUrl) .header("Destination", destinationUrl) .header("Overwrite", "T") @@ -364,6 +371,7 @@ internal class NextcloudDocumentWebDav( return execute( request = builder.method("MOVE", EMPTY_BODY).build(), operation = "replace file", + cancellation = cancellation, ) } catch (failure: Throwable) { runCatching { deleteOwnedStage(session, userId, stagingPath, stagedEtag) } diff --git a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentAtomicReplacementCancellationTest.kt b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentAtomicReplacementCancellationTest.kt new file mode 100644 index 000000000..0dd07e490 --- /dev/null +++ b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentAtomicReplacementCancellationTest.kt @@ -0,0 +1,134 @@ +package dev.obiente.nextcloudnative + +import dev.obiente.nextcloudnative.app.NextcloudSession +import java.nio.file.Files +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertTrue +import mockwebserver3.MockResponse +import mockwebserver3.MockWebServer +import okhttp3.Headers.Companion.headersOf + +class NextcloudDocumentAtomicReplacementCancellationTest { + @Test + fun `atomic replacement observes cancellation while staging`() { + MockWebServer().use { server -> + server.enqueue( + MockResponse.Builder() + .code(201) + .headers(headersOf("ETag", "\"staged-1\"")) + .headersDelay(30, TimeUnit.SECONDS) + .build(), + ) + server.enqueue(MockResponse.Builder().code(204).build()) + server.start() + val source = Files.createTempFile("ncn-cancel-replace-", ".txt").toFile() + val cancellation = TestCancellation() + val executor = Executors.newSingleThreadExecutor() + try { + source.writeText("edited") + val session = NextcloudSession(server.url("/").toString(), "alice", "secret") + val replacement = executor.submit { + NextcloudDocumentWebDav().replaceFileAtomically( + session, + "alice", + "Documents/report.txt", + source, + "\"old-1\"", + cancellation, + ) + } + + val upload = requireNotNull(server.takeRequest(2, TimeUnit.SECONDS)) + assertEquals("PUT", upload.method) + cancellation.cancel() + val failure = assertFailsWith { + replacement.get(2, TimeUnit.SECONDS) + } + assertTrue(failure.cause is TestCancelledException) + assertTrue(cancellation.detached.await(2, TimeUnit.SECONDS)) + val cleanup = requireNotNull(server.takeRequest(2, TimeUnit.SECONDS)) + assertEquals("DELETE", cleanup.method) + assertEquals(upload.url.encodedPath, cleanup.url.encodedPath) + } finally { + executor.shutdownNow() + source.delete() + } + } + } + + @Test + fun `atomic replacement cleans its stage when cancellation interrupts move`() { + MockWebServer().use { server -> + server.enqueue( + MockResponse.Builder().code(201) + .headers(headersOf("ETag", "\"staged-1\"")) + .build(), + ) + server.enqueue(MockResponse.Builder().code(201).headersDelay(30, TimeUnit.SECONDS).build()) + server.enqueue(MockResponse.Builder().code(204).build()) + server.start() + val source = Files.createTempFile("ncn-cancel-move-", ".txt").toFile() + val cancellation = TestCancellation() + val executor = Executors.newSingleThreadExecutor() + try { + source.writeText("edited") + val session = NextcloudSession(server.url("/").toString(), "alice", "secret") + val replacement = executor.submit { + NextcloudDocumentWebDav().replaceFileAtomically( + session, + "alice", + "Documents/report.txt", + source, + "\"old-1\"", + cancellation, + ) + } + + val upload = requireNotNull(server.takeRequest(2, TimeUnit.SECONDS)) + val move = requireNotNull(server.takeRequest(2, TimeUnit.SECONDS)) + assertEquals("PUT", upload.method) + assertEquals("MOVE", move.method) + cancellation.cancel() + val failure = assertFailsWith { + replacement.get(2, TimeUnit.SECONDS) + } + assertTrue(failure.cause is TestCancelledException) + val cleanup = requireNotNull(server.takeRequest(2, TimeUnit.SECONDS)) + assertEquals("DELETE", cleanup.method) + assertEquals(upload.url.encodedPath, cleanup.url.encodedPath) + } finally { + executor.shutdownNow() + source.delete() + } + } + } + + private class TestCancellation : DocumentRequestCancellation { + val attached = CountDownLatch(1) + val detached = CountDownLatch(1) + @Volatile private var cancelled = false + @Volatile private var cancelAction: (() -> Unit)? = null + + fun cancel() { + cancelled = true + cancelAction?.invoke() + } + + override fun throwIfCancelled() { + if (cancelled) throw TestCancelledException() + } + + override fun setOnCancelAction(action: (() -> Unit)?) { + cancelAction = action + if (action == null) detached.countDown() else attached.countDown() + if (cancelled) action?.invoke() + } + } + + private class TestCancelledException : RuntimeException() +} From 31f627ad61ba1e43f5cb514d8ae25526086826dd Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Fri, 4 Sep 2026 03:44:08 +0200 Subject: [PATCH 6/8] fix(android): reserve document mutation paths --- ...AndroidVirtualFileCacheInstrumentedTest.kt | 41 +++++++++++ .../AndroidDocumentWritebackRecovery.kt | 69 +++++++++++++++---- .../NextcloudDocumentsProvider.kt | 19 +++-- .../AndroidVirtualFileProxyCallbackTest.kt | 12 ++++ 4 files changed, 117 insertions(+), 24 deletions(-) diff --git a/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt index 5bc789d72..a1ae5df55 100644 --- a/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt +++ b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt @@ -7,6 +7,9 @@ import dev.obiente.nextcloudnative.app.NextcloudFile import dev.obiente.nextcloudnative.app.NextcloudSession import dev.obiente.nextcloudnative.app.VirtualFileCachePolicy import java.io.File +import java.util.concurrent.CountDownLatch +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit import mockwebserver3.MockResponse import mockwebserver3.MockWebServer import org.junit.After @@ -208,6 +211,44 @@ class AndroidVirtualFileCacheInstrumentedTest { } } + @Test + fun mutationReservationReleasesTheGlobalLockAndBlocksOnlyOverlappingEdits() { + val operationStarted = CountDownLatch(1) + val finishOperation = CountDownLatch(1) + val executor = Executors.newSingleThreadExecutor() + try { + val mutation = executor.submit { + withNoBlockingAndroidDocumentWriteback(context, session, "Projects/Active") { + operationStarted.countDown() + check(finishOperation.await(10, TimeUnit.SECONDS)) + } + } + org.junit.Assert.assertTrue(operationStarted.await(10, TimeUnit.SECONDS)) + + reserveAndroidDocumentWritebackPath(session, "Projects/Archive/notes.txt") + releaseAndroidDocumentWritebackPath(session, "Projects/Archive/notes.txt") + org.junit.Assert.assertThrows(IllegalStateException::class.java) { + reserveAndroidDocumentWritebackPath(session, "Projects/Active/notes.txt") + } + + finishOperation.countDown() + mutation.get(10, TimeUnit.SECONDS) + reserveAndroidDocumentWritebackPath(session, "Projects/Active/notes.txt") + releaseAndroidDocumentWritebackPath(session, "Projects/Active/notes.txt") + + org.junit.Assert.assertThrows(IllegalArgumentException::class.java) { + withNoBlockingAndroidDocumentWriteback(context, session, "Projects/Failed") { + throw IllegalArgumentException("synthetic mutation failure") + } + } + reserveAndroidDocumentWritebackPath(session, "Projects/Failed/notes.txt") + releaseAndroidDocumentWritebackPath(session, "Projects/Failed/notes.txt") + } finally { + finishOperation.countDown() + executor.shutdownNow() + } + } + @Test fun providerStartupDiscardsIncompleteWritebacksAndKeepsReadyRecovery() { val recovery = File(context.filesDir, "documents-recovery").apply { mkdirs() } diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt index 41b9e29d1..627175697 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidDocumentWritebackRecovery.kt @@ -167,7 +167,9 @@ internal fun claimAndroidDocumentPendingWritebackForRecovery( session: NextcloudSession, remotePath: String, ): AndroidDocumentPendingWriteback? = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val activePath = ActiveAndroidDocumentWritebackPath(NextcloudDocumentIds.accountKey(session), remotePath) + val accountId = NextcloudDocumentIds.accountKey(session) + if (androidDocumentMutationBlocksWriteback(accountId, remotePath)) return null + val activePath = ActiveAndroidDocumentWritebackPath(accountId, remotePath) if (!ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.add(activePath)) return null val pending = androidDocumentPendingWriteback(context, session, remotePath) if (pending == null) { @@ -180,7 +182,11 @@ internal fun claimAndroidDocumentPendingWritebackForRecovery( internal fun reserveAndroidDocumentWritebackPath(session: NextcloudSession, remotePath: String) = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val active = ActiveAndroidDocumentWritebackPath(NextcloudDocumentIds.accountKey(session), remotePath) + val accountId = NextcloudDocumentIds.accountKey(session) + check(!androidDocumentMutationBlocksWriteback(accountId, remotePath)) { + "This document is already being changed by another local operation." + } + val active = ActiveAndroidDocumentWritebackPath(accountId, remotePath) check(ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.add(active)) { "This document already has an active local edit." } @@ -197,19 +203,37 @@ internal fun withNoBlockingAndroidDocumentWriteback( session: NextcloudSession, vararg remotePaths: String, operation: () -> T, -): T = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { - val accountId = NextcloudDocumentIds.accountKey(session) - val activePaths = ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.asSequence() - .filter { active -> active.accountId == accountId } - .map(ActiveAndroidDocumentWritebackPath::remotePath) +): T { val providerContext = requireNotNull(context) { "Provider context is unavailable." } - val retainedPaths = androidDocumentPendingWritebacks(providerContext, session) - .asSequence() - .map(AndroidDocumentPendingWriteback::remotePath) - check(!androidDocumentWritebacksBlockMutation(activePaths, retainedPaths, *remotePaths)) { - "This document cannot be changed while a local edit still needs recovery." + val accountId = NextcloudDocumentIds.accountKey(session) + val paths = remotePaths.toSet() + require(paths.isNotEmpty() && paths.none(String::isBlank)) + val reservation = synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + val activePaths = ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS.asSequence() + .filter { active -> active.accountId == accountId } + .map(ActiveAndroidDocumentWritebackPath::remotePath) + val retainedPaths = androidDocumentPendingWritebacks(providerContext, session) + .asSequence() + .map(AndroidDocumentPendingWriteback::remotePath) + check(!androidDocumentWritebacksBlockMutation(activePaths, retainedPaths, *remotePaths)) { + "This document cannot be changed while a local edit still needs recovery." + } + check( + ACTIVE_ANDROID_DOCUMENT_MUTATIONS.none { active -> + active.accountId == accountId && androidDocumentMutationPathsOverlap(active.remotePaths, paths) + }, + ) { + "This document is already being changed by another local operation." + } + ActiveAndroidDocumentMutation(accountId, paths).also(ACTIVE_ANDROID_DOCUMENT_MUTATIONS::add) + } + return try { + operation() + } finally { + synchronized(ANDROID_DOCUMENT_WRITEBACK_LOCK) { + check(ACTIVE_ANDROID_DOCUMENT_MUTATIONS.remove(reservation)) + } } - operation() } internal fun androidDocumentWritebacksBlockMutation( @@ -225,6 +249,19 @@ internal fun androidDocumentWritebackPathBlocksMutation( vararg mutationPaths: String, ): Boolean = mutationPaths.any { path -> activePath == path || activePath.startsWith("$path/") } +internal fun androidDocumentMutationPathsOverlap(first: Set, second: Set): Boolean = + first.any { left -> + second.any { right -> + left == right || left.startsWith("$right/") || right.startsWith("$left/") + } + } + +private fun androidDocumentMutationBlocksWriteback(accountId: String, remotePath: String): Boolean = + ACTIVE_ANDROID_DOCUMENT_MUTATIONS.any { active -> + active.accountId == accountId && + androidDocumentWritebackPathBlocksMutation(remotePath, *active.remotePaths.toTypedArray()) + } + internal inline fun handleAndroidDocumentWritebackRecoveryFailure( failure: Throwable, release: () -> Unit, @@ -333,7 +370,13 @@ private data class ActiveAndroidDocumentWritebackPath( val remotePath: String, ) +private class ActiveAndroidDocumentMutation( + val accountId: String, + val remotePaths: Set, +) + private val ANDROID_DOCUMENT_WRITEBACK_LOCK = Any() private val ACTIVE_ANDROID_DOCUMENT_WRITEBACKS = ConcurrentHashMap.newKeySet() private val ACTIVE_ANDROID_DOCUMENT_WRITEBACK_PATHS = ConcurrentHashMap.newKeySet() +private val ACTIVE_ANDROID_DOCUMENT_MUTATIONS = mutableSetOf() diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt index 13379d102..02d758a6f 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt @@ -467,17 +467,14 @@ class NextcloudDocumentsProvider : DocumentsProvider() { val parent = requireReference(parentDocumentId, session) val account = resolveAccount(session) requireDirectory(session, account, parent) - val safeName = requireSafeDisplayName(displayName) - val path = childPath(parent.path, safeName) - mutationCall { - if (mimeType == DocumentsContract.Document.MIME_TYPE_DIR) { - webDav.createFolder(session, account.userId, path) - } else { - val empty = createLocalStagingFile() - try { - webDav.createFile(session, account.userId, path, empty) - } finally { - empty.delete() + val path = childPath(parent.path, requireSafeDisplayName(displayName)) + withNoBlockingAndroidDocumentWriteback(context, session, path) { + mutationCall { + if (mimeType == DocumentsContract.Document.MIME_TYPE_DIR) { + webDav.createFolder(session, account.userId, path) + } else { + val empty = createLocalStagingFile() + try { webDav.createFile(session, account.userId, path, empty) } finally { empty.delete() } } } } diff --git a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt index 285b5e0df..8326f95f5 100644 --- a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt +++ b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileProxyCallbackTest.kt @@ -119,6 +119,18 @@ class AndroidVirtualFileProxyCallbackTest { "Projects/Archive", ), ) + assertTrue( + androidDocumentMutationPathsOverlap( + setOf("Projects/Active"), + setOf("Projects/Active/notes.txt"), + ), + ) + assertFalse( + androidDocumentMutationPathsOverlap( + setOf("Projects/Active"), + setOf("Projects/Archive"), + ), + ) } @Test From 391e2ee2bc8f378a1330c3974fc5512390a9bfb5 Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Fri, 4 Sep 2026 04:10:31 +0200 Subject: [PATCH 7/8] fix(android): bound staged upload cleanup --- .../NextcloudDocumentWebDav.kt | 28 ++++++++++++------- ...cumentAtomicReplacementCancellationTest.kt | 9 ++++-- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentWebDav.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentWebDav.kt index 22a3458a3..1ca65f49d 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentWebDav.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentWebDav.kt @@ -454,7 +454,11 @@ internal class NextcloudDocumentWebDav( ) { val builder = requestBuilder(session, buildNextcloudFileUrl(session.serverUrl, userId, path)) expectedEtag?.takeIf(String::isNotBlank)?.let { builder.header("If-Match", it) } - execute(builder.delete().build(), "clean up staged upload") + execute( + request = builder.delete().build(), + operation = "clean up staged upload", + callTimeoutMillis = OWNED_STAGE_CLEANUP_TIMEOUT_MILLIS, + ) } internal fun execute( @@ -463,21 +467,24 @@ internal class NextcloudDocumentWebDav( onRequestStarted: () -> Unit = {}, cancellation: DocumentRequestCancellation = NoDocumentRequestCancellation, timeoutMillis: Long? = null, + callTimeoutMillis: Long? = null, requiredSuccessStatus: Int? = null, ): DocumentMutationResult { check(cloudMutationsAllowed()) { "This emulator is using a shared read-only test session. Cloud changes are blocked." } cancellation.throwIfCancelled() - val operationClient = timeoutMillis?.let { timeout -> - require(timeout > 0L) - client.newBuilder() - .readTimeout(timeout, TimeUnit.MILLISECONDS) - .writeTimeout(timeout, TimeUnit.MILLISECONDS) - .callTimeout(0L, TimeUnit.MILLISECONDS) - .build() - } ?: client - val requestClient = operationClient.newBuilder() + require(timeoutMillis == null || timeoutMillis > 0L) + require(callTimeoutMillis == null || callTimeoutMillis > 0L) + val requestClient = client.newBuilder() + .apply { + timeoutMillis?.let { timeout -> + readTimeout(timeout, TimeUnit.MILLISECONDS) + writeTimeout(timeout, TimeUnit.MILLISECONDS) + callTimeout(0L, TimeUnit.MILLISECONDS) + } + callTimeoutMillis?.let { timeout -> callTimeout(timeout, TimeUnit.MILLISECONDS) } + } .eventListener( object : EventListener() { override fun requestHeadersStart(call: Call) { @@ -570,6 +577,7 @@ internal class NextcloudDocumentWebDav( const val DEFAULT_DIRECTORY_ENTRY_LIMIT = 1_000 const val MAX_DIRECTORY_ENTRY_LIMIT = 5_000 const val MAX_DIRECTORY_RESPONSE_BYTES = 4L * 1024L * 1024L + const val OWNED_STAGE_CLEANUP_TIMEOUT_MILLIS = 3_000L val OCTET_STREAM = "application/octet-stream".toMediaType() val XML_CONTENT_TYPE = "application/xml; charset=utf-8".toMediaType() val EMPTY_BODY = byteArrayOf().toRequestBody(null) diff --git a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentAtomicReplacementCancellationTest.kt b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentAtomicReplacementCancellationTest.kt index 0dd07e490..52d30e355 100644 --- a/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentAtomicReplacementCancellationTest.kt +++ b/androidApp/src/test/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentAtomicReplacementCancellationTest.kt @@ -24,7 +24,12 @@ class NextcloudDocumentAtomicReplacementCancellationTest { .headersDelay(30, TimeUnit.SECONDS) .build(), ) - server.enqueue(MockResponse.Builder().code(204).build()) + server.enqueue( + MockResponse.Builder() + .code(204) + .headersDelay(30, TimeUnit.SECONDS) + .build(), + ) server.start() val source = Files.createTempFile("ncn-cancel-replace-", ".txt").toFile() val cancellation = TestCancellation() @@ -47,7 +52,7 @@ class NextcloudDocumentAtomicReplacementCancellationTest { assertEquals("PUT", upload.method) cancellation.cancel() val failure = assertFailsWith { - replacement.get(2, TimeUnit.SECONDS) + replacement.get(6, TimeUnit.SECONDS) } assertTrue(failure.cause is TestCancelledException) assertTrue(cancellation.detached.await(2, TimeUnit.SECONDS)) From 1c122470f1bd0254c8e321cdd5584a2912fe4163 Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Fri, 4 Sep 2026 04:40:39 +0200 Subject: [PATCH 8/8] fix(android): guard native text saves --- ...AndroidVirtualFileCacheInstrumentedTest.kt | 37 +++++++++++++++++++ .../AndroidNextcloudServices.kt | 35 ++++++++++-------- 2 files changed, 57 insertions(+), 15 deletions(-) diff --git a/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt index a1ae5df55..ff5b6f0e2 100644 --- a/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt +++ b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidVirtualFileCacheInstrumentedTest.kt @@ -10,6 +10,7 @@ import java.io.File import java.util.concurrent.CountDownLatch import java.util.concurrent.Executors import java.util.concurrent.TimeUnit +import kotlinx.coroutines.runBlocking import mockwebserver3.MockResponse import mockwebserver3.MockWebServer import org.junit.After @@ -17,6 +18,7 @@ import org.junit.Assert.assertArrayEquals import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -164,6 +166,41 @@ class AndroidVirtualFileCacheInstrumentedTest { org.junit.Assert.assertTrue(unrelatedMutationRan) } + @Test + fun processRestoredWritebackBlocksNativeTextSaveUntilRecovery() { + MockWebServer().use { server -> + server.start() + val saveSession = session.copy(serverUrl = server.url("/").toString()) + val recovery = File(context.filesDir, "documents-recovery").apply { mkdirs() } + val stage = File(recovery, "writeback-text-save.stage").apply { writeText("local edit") } + File(recovery, stage.name + ".json").writeText( + JSONObject() + .put("version", 1) + .put("account", NextcloudDocumentIds.accountKey(saveSession)) + .put("path", "Notes/draft.md") + .put("etag", "\"v1\"") + .put("displayName", "draft.md") + .put("stage", stage.name) + .put("startedAt", 10L) + .put("ready", true) + .toString(), + ) + + assertThrows(IllegalStateException::class.java) { + runBlocking { + AndroidNextcloudServices(context).saveTextFile( + session = saveSession, + userId = "virtual-cache-fixture", + path = "Notes/draft.md", + text = "native editor update", + expectedEtag = "\"v1\"", + ) + } + } + assertEquals(0, server.requestCount) + } + } + @Test fun processRestoredWritebackBlocksFileSyncRemoteDeletion() { MockWebServer().use { server -> diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt index a8087d2fe..99ad45d7b 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt @@ -35,7 +35,6 @@ import dev.obiente.nextcloudnative.app.normalizeServerUrl import dev.obiente.nextcloudnative.app.toApprovedDiagnostic import dev.obiente.nextcloudnative.app.toStartedDiagnostic import dev.obiente.nextcloudnative.app.confirmTextFileDavSave -import dev.obiente.nextcloudnative.app.runCatchingPreservingCancellation import dev.obiente.nextcloudnative.app.textFileDavSaveRequest import dev.obiente.nextcloudnative.app.MAX_EDITABLE_TEXT_BYTES import dev.obiente.nextcloudnative.app.MAX_FILE_IDENTITY_SEARCH_BATCH @@ -2672,20 +2671,26 @@ internal class AndroidNextcloudServices( text: String, expectedEtag: String, ): SavedTextFile = withContext(Dispatchers.IO) { - val specification = textFileDavSaveRequest(text, expectedEtag) - val response = request( - method = "PUT", - url = buildNextcloudFileUrl(session.serverUrl, userId, path), - session = session, - rawBody = specification.body, - contentType = specification.contentType, - headers = specification.headers, - ) - val confirmation = confirmTextFileDavSave(response.status) - val etag = response.etag ?: - runCatchingPreservingCancellation { loadFileEtag(session, userId, path) }.getOrNull() - runCatching { fileReadCache.invalidate(NextcloudDocumentIds.accountKey(session), path) } - SavedTextFile(etag, confirmation.created) + withNoBlockingAndroidDocumentWriteback(appContext, session, path) { + val specification = textFileDavSaveRequest(text, expectedEtag) + val response = request( + method = "PUT", + url = buildNextcloudFileUrl(session.serverUrl, userId, path), + session = session, + rawBody = specification.body, + contentType = specification.contentType, + headers = specification.headers, + ) + val confirmation = confirmTextFileDavSave(response.status) + val etag = response.etag ?: try { + loadFileEtag(session, userId, path) + } catch (failure: Exception) { + if (failure is CancellationException) throw failure + null + } + runCatching { fileReadCache.invalidate(NextcloudDocumentIds.accountKey(session), path) } + SavedTextFile(etag, confirmation.created) + } } override suspend fun createTextFileIfAbsent(