Recover corrupt versioned stores instead of crashing

kstore-file/src/commonMain/kotlin/io/github/xxfast/kstore/file/extensions/KVersionedStore.kt

@@ -3,11 +3,13 @@ package io.github.xxfast.kstore.file.extensions
 import io.github.xxfast.kstore.Codec
 import io.github.xxfast.kstore.DefaultJson
 import io.github.xxfast.kstore.KStore
+import io.github.xxfast.kstore.file.moveOrCopy
 import io.github.xxfast.kstore.storeOf
 import kotlinx.io.buffered
 import kotlinx.io.files.FileNotFoundException
 import kotlinx.io.files.Path
 import kotlinx.io.files.SystemFileSystem
+import kotlinx.serialization.DeserializationStrategy
 import kotlinx.serialization.ExperimentalSerializationApi
 import kotlinx.serialization.KSerializer
 import kotlinx.serialization.Serializable
@@ -63,22 +65,55 @@ public class VersionedCodec<T : @Serializable Any>(
   private val tempVersionPath: Path = Path("$versionPath.temp"),
 ) : Codec<T> {
 
+  /**
+   * Decodes the file to a value.
+   * If the file does not exist, null is returned.
+   * If the file does not hold the current shape of [T], the value is recovered through [migration].
+   * @return optional value that is decoded
+   */
   override suspend fun decode(): T? =
     try {
       SystemFileSystem.source(file).buffered().use { json.decode(serializer, it) }
+    } catch (e: FileNotFoundException) {
+      null
     } catch (e: SerializationException) {
-      val previousVersion: Int =
-        if (SystemFileSystem.exists(versionPath))
-          SystemFileSystem.source(versionPath).buffered().use { json.decode(Int.serializer(), it) }
-        else 0
+      // The file doesn't hold the current shape of [T]. Either it was written by an older version of
+      // this store - which [migration] can recover from - or it is corrupt/partially written, in which
+      // case there is nothing to recover and [migration] is handed what little is known.
+      // No version file at all means the store predates versioning, so it reads as 0. One that exists
+      // but can't be read leaves the version unknown, same as unreadable data.
+      migration(
+        decodeOrNull(versionPath, Int.serializer(), whenMissing = 0),
+        decodeOrNull(file, JsonElement.serializer(), whenMissing = null),
+      )
+    }
 
-      val data: JsonElement =
-        SystemFileSystem.source(file).buffered().use { json.decode(it) }
-      migration(previousVersion, data)
+  /**
+   * Reads [path] with [deserializer], degrading rather than throwing so that a corrupt store can be
+   * migrated or reset instead of crashing on every read.
+   * @return [whenMissing] when there is no such file, or null when its contents cannot be decoded
+   */
+  private fun <R : Any> decodeOrNull(
+    path: Path,
+    deserializer: DeserializationStrategy<R>,
+    whenMissing: R?,
+  ): R? =
+    try {
+      SystemFileSystem.source(path).buffered().use { json.decode(deserializer, it) }
     } catch (e: FileNotFoundException) {
+      whenMissing
+    } catch (e: SerializationException) {
       null
     }
 
+  /**
+   * Encodes the given value to the file, along with the current [version].
+   * If the value is null, both files are deleted.
+   * If the encoding fails, the temp files are deleted.
+   * 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
+   */
   override suspend fun encode(value: T?) {
     if (value == null) {
       SystemFileSystem.delete(versionPath, mustExist = false)
@@ -95,7 +130,9 @@ public class VersionedCodec<T : @Serializable Any>(
       throw e
     }
 
-    SystemFileSystem.atomicMove(source = tempPath, destination = file)
-    SystemFileSystem.atomicMove(source = tempVersionPath, destination = versionPath)
+    // Data first, version second. A crash in between leaves the new data with a stale version, which
+    // still decodes directly. The reverse order would claim a version the data hasn't been written to.
+    moveOrCopy(source = tempPath, destination = file)
+    moveOrCopy(source = tempVersionPath, destination = versionPath)
   }
 }

kstore-file/src/commonTest/kotlin/io/github/xxfast/kstore/file/extensions/KVersionedStoreTests.kt

@@ -3,8 +3,10 @@ package io.github.xxfast.kstore.file.extensions
 import io.github.xxfast.kstore.KStore
 import io.github.xxfast.kstore.file.storeOf
 import kotlinx.coroutines.test.runTest
+import kotlinx.io.buffered
 import kotlinx.io.files.Path
 import kotlinx.io.files.SystemFileSystem
+import kotlinx.io.writeString
 import kotlinx.serialization.KSerializer
 import kotlinx.serialization.Serializable
 import kotlinx.serialization.encoding.Decoder
@@ -48,6 +50,11 @@ val MYLO_V42 = CatV42(name = "mylo", friends = mapOf("oreo" to 5, "kat" to 10))
 
 class KVersionedStoreTests {
   private val file: Path = Path("test_migration.json")
+  private val versionFile: Path = Path("$file.version")
+
+  private fun write(path: Path, contents: String) {
+    SystemFileSystem.sink(path).buffered().use { it.writeString(contents) }
+  }
 
   private val storeV0: KStore<CatV0> = storeOf(file = file)
 
@@ -101,7 +108,7 @@ class KVersionedStoreTests {
   @AfterTest
   fun cleanup() {
     SystemFileSystem.delete(file, mustExist = false)
-    SystemFileSystem.delete(Path("${file.name}.version"), mustExist = false)
+    SystemFileSystem.delete(versionFile, mustExist = false)
     SystemFileSystem.delete(Path("${file.name}.temp"), mustExist = false)
     SystemFileSystem.delete(Path("${file.name}.version.temp"), mustExist = false)
   }
@@ -155,6 +162,54 @@ class KVersionedStoreTests {
     assertEquals(expect, actual)
   }
 
+  // Corrupt or partially written stores must decode to null rather than crash - see issues #80, #157, #162
+
+  @Test
+  fun testDecodeEmptyFileWithEmptyVersionFile() = runTest {
+    write(file, "")
+    write(versionFile, "")
+    assertEquals(null, storeV2.get())
+  }
+
+  @Test
+  fun testDecodeEmptyFileWithVersionFile() = runTest {
+    write(file, "")
+    write(versionFile, "1")
+    assertEquals(null, storeV2.get())
+  }
+
+  @Test
+  fun testDecodeTruncatedFile() = runTest {
+    write(file, """{"name":"mylo","liv""")
+    write(versionFile, "1")
+    assertEquals(null, storeV2.get())
+  }
+
+  @Test
+  fun testDecodeEmptyFileWithoutVersionFile() = runTest {
+    write(file, "")
+    assertEquals(null, storeV2.get())
+  }
+
+  @Test
+  fun testCorruptStoreRepairsOnNextWrite() = runTest {
+    write(file, "")
+    write(versionFile, "")
+    assertEquals(null, storeV2.get())
+
+    storeV2.set(MYLO_V2)
+    assertEquals(MYLO_V2, storeV2.get())
+  }
+
+  @Test
+  fun testMigrationWithUnreadableVersionFile() = runTest {
+    // The data is intact and from v1, but the version file didn't survive. The version is unknown,
+    // so the migration can't place the data and falls through to its else branch rather than crashing.
+    storeV1.set(MYLO_V1)
+    write(versionFile, "")
+    assertEquals(null, storeV2.get())
+  }
+
   @Test
   fun testTransactionalEncode() = runTest {
     assertFailsWith<NotImplementedError> { storeV41.set(MYLO_V41) }