From 3c9265878a9fe3ff267bf9765178cc3d7b5ba836 Mon Sep 17 00:00:00 2001 From: THEAccess Date: Tue, 26 Aug 2025 00:10:33 +0200 Subject: [PATCH 1/2] Implement copy-and-delete fallback for atomicMove This change introduces a fallback mechanism for `atomicMove` in `FileCodec.kt`. If `atomicMove` is unsupported (e.g., on Android 7 and below), it now attempts to copy the temporary file to the destination and then deletes the temporary file. Error handling is included to ensure the temporary file is cleaned up in case of a copy failure. --- .../io/github/xxfast/kstore/file/FileCodec.kt | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/kstore-file/src/commonMain/kotlin/io/github/xxfast/kstore/file/FileCodec.kt b/kstore-file/src/commonMain/kotlin/io/github/xxfast/kstore/file/FileCodec.kt index b5c0bfa..7e13bb1 100644 --- a/kstore-file/src/commonMain/kotlin/io/github/xxfast/kstore/file/FileCodec.kt +++ b/kstore-file/src/commonMain/kotlin/io/github/xxfast/kstore/file/FileCodec.kt @@ -57,6 +57,7 @@ public class FileCodec( * If the value is null, the file is deleted. * If the encoding fails, the temp file is deleted. * If the encoding succeeds, the temp file is atomically moved to the target file - completing the transaction. + * Uses fallback copy-and-delete for platforms where atomic move is not supported (e.g., Android 7 and below). * @param value optional value to encode */ @OptIn(ExperimentalSerializationApi::class) @@ -73,6 +74,25 @@ public class FileCodec( throw e } - SystemFileSystem.atomicMove(source = tempFile, destination = file) + // Try atomic move first, fallback to copy-and-delete if not supported + try { + SystemFileSystem.atomicMove(source = tempFile, destination = file) + } catch (_: UnsupportedOperationException) { + // Fallback for platforms where atomic move is not supported (e.g., Android 7 and below) + try { + // Copy temp file to destination + SystemFileSystem.source(tempFile).buffered().use { source -> + SystemFileSystem.sink(file).buffered().use { destination -> + source.transferTo(destination) + } + } + // Delete temp file after successful copy + SystemFileSystem.delete(tempFile, mustExist = false) + } catch (copyException: Throwable) { + // Clean up temp file and rethrow + SystemFileSystem.delete(tempFile, mustExist = false) + throw copyException + } + } } } From 9b17f888813692af8487b65ca6f304329a52df5c Mon Sep 17 00:00:00 2001 From: xxfast Date: Sun, 28 Jun 2026 02:21:40 +1000 Subject: [PATCH 2/2] Add additional test cases to increase coverage --- .../io/github/xxfast/kstore/file/FileCodec.kt | 48 ++++++++++++------- .../xxfast/kstore/file/FileCodecTests.kt | 37 ++++++++++++++ 2 files changed, 68 insertions(+), 17 deletions(-) diff --git a/kstore-file/src/commonMain/kotlin/io/github/xxfast/kstore/file/FileCodec.kt b/kstore-file/src/commonMain/kotlin/io/github/xxfast/kstore/file/FileCodec.kt index 7e13bb1..3aff99e 100644 --- a/kstore-file/src/commonMain/kotlin/io/github/xxfast/kstore/file/FileCodec.kt +++ b/kstore-file/src/commonMain/kotlin/io/github/xxfast/kstore/file/FileCodec.kt @@ -57,7 +57,8 @@ public class FileCodec( * If the value is null, the file is deleted. * If the encoding fails, the temp file is deleted. * If the encoding succeeds, the temp file is atomically moved to the target file - completing the transaction. - * Uses fallback copy-and-delete for platforms where atomic move is not supported (e.g., Android 7 and below). + * On platforms where atomic move is not supported (e.g., Android 7 and below) this falls back to a + * non-atomic copy-and-delete; the transactional guarantee does not hold for that fallback path. * @param value optional value to encode */ @OptIn(ExperimentalSerializationApi::class) @@ -74,25 +75,38 @@ public class FileCodec( throw e } - // Try atomic move first, fallback to copy-and-delete if not supported + moveOrCopy(source = tempFile, destination = file) + } +} + +/** + * Moves [source] onto [destination] atomically via [atomicMove]. + * + * On platforms where atomic move is unsupported (e.g., Android 7 and below - see issue #137) this falls + * back to a non-atomic copy-and-delete. The fallback is not transactional: a crash mid-copy can leave the + * destination partially written. On a failed copy the (possibly corrupt) destination is removed. + * + * The [atomicMove] parameter is a seam over SystemFileSystem.atomicMove so the fallback path can be exercised in tests. + */ +internal fun moveOrCopy( + source: Path, + destination: Path, + atomicMove: (source: Path, destination: Path) -> Unit = SystemFileSystem::atomicMove, +) { + try { + atomicMove(source, destination) + } catch (_: UnsupportedOperationException) { try { - SystemFileSystem.atomicMove(source = tempFile, destination = file) - } catch (_: UnsupportedOperationException) { - // Fallback for platforms where atomic move is not supported (e.g., Android 7 and below) - try { - // Copy temp file to destination - SystemFileSystem.source(tempFile).buffered().use { source -> - SystemFileSystem.sink(file).buffered().use { destination -> - source.transferTo(destination) - } + SystemFileSystem.source(source).buffered().use { from -> + SystemFileSystem.sink(destination).buffered().use { to -> + from.transferTo(to) } - // Delete temp file after successful copy - SystemFileSystem.delete(tempFile, mustExist = false) - } catch (copyException: Throwable) { - // Clean up temp file and rethrow - SystemFileSystem.delete(tempFile, mustExist = false) - throw copyException } + } catch (e: Throwable) { + SystemFileSystem.delete(destination, mustExist = false) + throw e + } finally { + SystemFileSystem.delete(source, mustExist = false) } } } diff --git a/kstore-file/src/commonTest/kotlin/io/github/xxfast/kstore/file/FileCodecTests.kt b/kstore-file/src/commonTest/kotlin/io/github/xxfast/kstore/file/FileCodecTests.kt index c96a31c..e376b33 100644 --- a/kstore-file/src/commonTest/kotlin/io/github/xxfast/kstore/file/FileCodecTests.kt +++ b/kstore-file/src/commonTest/kotlin/io/github/xxfast/kstore/file/FileCodecTests.kt @@ -3,6 +3,7 @@ package io.github.xxfast.kstore.file import io.github.xxfast.kstore.DefaultJson import kotlinx.coroutines.test.runTest import kotlinx.io.buffered +import kotlinx.io.files.FileNotFoundException import kotlinx.io.files.Path import kotlinx.io.files.SystemFileSystem import kotlinx.io.writeString @@ -37,6 +38,7 @@ class FileCodecTests { @AfterTest fun cleanUp() { SystemFileSystem.delete(Path(FILE_PATH), false) + SystemFileSystem.delete(Path("$FILE_PATH.temp"), false) } @Test @@ -80,4 +82,39 @@ class FileCodecTests { SystemFileSystem.sink(Path(FILE_PATH)).buffered().use { it.writeString("💩") } assertFailsWith { codec.decode() } } + + @Test + fun testMoveFallsBackToCopyWhenAtomicMoveUnsupported() = runTest { + val tempFile = Path("$FILE_PATH.temp") + SystemFileSystem.sink(tempFile).buffered().use { DefaultJson.encodeToSink>(listOf(MYLO), it) } + + // Simulate platforms (e.g. Android 7 and below) where atomicMove throws - see issue #137 + moveOrCopy( + source = tempFile, + destination = Path(FILE_PATH), + atomicMove = { _, _ -> throw UnsupportedOperationException("Atomic move not supported") }, + ) + + // The value is persisted via the copy-and-delete fallback ... + assertEquals(listOf(MYLO), stored) + // ... and the temp file is cleaned up afterwards. + assertEquals(false, SystemFileSystem.exists(tempFile)) + } + + @Test + fun testMoveDeletesCorruptDestinationWhenCopyFails() = runTest { + val tempFile = Path("$FILE_PATH.temp") // intentionally never created, so the fallback copy fails + SystemFileSystem.sink(Path(FILE_PATH)).buffered().use { it.writeString("stale") } // pre-existing destination + + assertFailsWith { + moveOrCopy( + source = tempFile, + destination = Path(FILE_PATH), + atomicMove = { _, _ -> throw UnsupportedOperationException("Atomic move not supported") }, + ) + } + + // The possibly-corrupt destination is removed rather than left half-written. + assertEquals(false, SystemFileSystem.exists(Path(FILE_PATH))) + } }