From 94aa9a070b67e7b6035eae1eda47078567dcb3b4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 12:28:06 +0000 Subject: [PATCH] fix(cordn): recursive delete carries on past a directory it cannot list deleteRecursivelyQuietly (the okio stand-in for File.deleteRecursively) listed the whole tree with listRecursively(...).toList() inside one try: a single directory that failed to list emptied the list, so nothing was deleted. File.deleteRecursively removed everything it could reach. For the cordn stores that meant a deleted group could keep its plaintext message history, and a migration could merge an imported snapshot into old state. It now lists one directory at a time, so an unlistable directory costs only its own subtree and the result is false. metadataOrNull does not follow symlinks, so a link is still deleted rather than walked into. FileSystemExtTest covers a whole tree, a missing path, a directory that refuses to list (failed before this change: the sibling's messages file survived) and a symlink whose target must survive. Also corrects EncryptedAppendLog's cache-key comment: Path.normalized() does not resolve a relative path against the working directory, unlike File.absolutePath. Every store builds its paths from one directory, so no caller mixes spellings. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01S7FuNBSKiyVecARSoE4B9P --- .../commons/storage/EncryptedAppendLog.kt | 6 +- .../amethyst/commons/util/FileSystemExt.kt | 24 ++-- .../commons/util/FileSystemExtTest.kt | 103 ++++++++++++++++++ 3 files changed, 123 insertions(+), 10 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/util/FileSystemExtTest.kt diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/storage/EncryptedAppendLog.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/storage/EncryptedAppendLog.kt index a7d4dc3726..26b85d4b1d 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/storage/EncryptedAppendLog.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/storage/EncryptedAppendLog.kt @@ -100,7 +100,11 @@ class EncryptedAppendLog( var looseEntries: Int, ) - /** Keyed by the normalized path, so two spellings of one file share an entry. */ + /** + * Keyed by the normalized path. That folds `a/./b` and `a/../a/b` together but does not resolve + * a relative path against the working directory, so callers must spell one file one way; every + * store here builds its paths from one directory. + */ private val logs = mutableMapOf() private fun stateFor(file: Path): LogState = diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/util/FileSystemExt.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/util/FileSystemExt.kt index 0427a556b3..0348957608 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/util/FileSystemExt.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/util/FileSystemExt.kt @@ -61,16 +61,22 @@ fun FileSystem.deleteQuietly(path: Path): Boolean = * @return true when nothing is left, including when [path] never existed. */ fun FileSystem.deleteRecursivelyQuietly(path: Path): Boolean { - // Directories come before their children in this listing, so the reverse - // deletes every child before the directory holding it. - val children = - try { - listRecursively(path).toList() - } catch (e: IOException) { - emptyList() - } var deleted = true - for (child in children.asReversed()) deleted = deleteQuietly(child) && deleted + // metadataOrNull does not follow symlinks, so a link reads as a non-directory + // and is deleted itself rather than walked into. + if (metadataOrNull(path)?.isDirectory == true) { + // Listed one directory at a time, so a directory that cannot be listed + // costs only its own subtree: its siblings are still deleted, as + // File.deleteRecursively did. + val children = + try { + list(path) + } catch (e: IOException) { + deleted = false + emptyList() + } + for (child in children) deleted = deleteRecursivelyQuietly(child) && deleted + } return deleteQuietly(path) && deleted } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/util/FileSystemExtTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/util/FileSystemExtTest.kt new file mode 100644 index 0000000000..3f0d16a473 --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/util/FileSystemExtTest.kt @@ -0,0 +1,103 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.util + +import okio.FileSystem +import okio.ForwardingFileSystem +import okio.IOException +import okio.Path +import okio.Path.Companion.toOkioPath +import java.nio.file.Files +import kotlin.test.AfterTest +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +class FileSystemExtTest { + private val root = Files.createTempDirectory("fs-ext-test").toFile() + + @AfterTest + fun cleanup() { + root.deleteRecursively() + } + + /** Refuses to list one directory, as an unreadable or concurrently removed one would. */ + private class UnlistableDirFileSystem( + private val unlistable: Path, + ) : ForwardingFileSystem(FileSystem.SYSTEM) { + override fun list(dir: Path): List { + if (dir == unlistable) throw IOException("cannot list $dir") + return super.list(dir) + } + } + + private fun write( + relative: String, + text: String = "x", + ) { + val file = root.resolve(relative) + file.parentFile.mkdirs() + file.writeText(text) + } + + @Test + fun deletesAWholeTree() { + write("a/b/c.txt") + write("a/d.txt") + val dir = root.resolve("a").toOkioPath() + + assertTrue(FileSystem.SYSTEM.deleteRecursivelyQuietly(dir)) + assertFalse(FileSystem.SYSTEM.exists(dir)) + } + + @Test + fun missingPathCountsAsDeleted() { + assertTrue(FileSystem.SYSTEM.deleteRecursivelyQuietly(root.resolve("never").toOkioPath())) + } + + @Test + fun carriesOnPastADirectoryItCannotList() { + // File.deleteRecursively removed everything it could reach and reported false for the + // rest. One unlistable subdirectory must not stop its siblings from being deleted. + write("g/stuck/inner.txt") + write("g/sibling/messages", "plaintext") + write("g/state") + val dir = root.resolve("g").toOkioPath() + val fs = UnlistableDirFileSystem(dir / "stuck") + + assertFalse(fs.deleteRecursivelyQuietly(dir)) + assertFalse(root.resolve("g/sibling/messages").exists()) + assertFalse(root.resolve("g/state").exists()) + assertTrue(root.resolve("g/stuck/inner.txt").exists()) + } + + @Test + fun deletesASymlinkWithoutEmptyingItsTarget() { + write("outside/keep.txt") + write("h/own.txt") + Files.createSymbolicLink(root.resolve("h/link").toPath(), root.resolve("outside").toPath()) + val dir = root.resolve("h").toOkioPath() + + assertTrue(FileSystem.SYSTEM.deleteRecursivelyQuietly(dir)) + assertFalse(root.resolve("h").exists()) + assertTrue(root.resolve("outside/keep.txt").exists()) + } +}