Fix crash: IllegalStateException: Unsupported concurrent change during composition

app/src/main/java/com/ethran/notable/data/PageDataManager.kt

@@ -8,6 +8,7 @@ import android.graphics.Bitmap
 import android.graphics.Rect
 import android.os.FileObserver
 import androidx.compose.runtime.mutableStateMapOf
+import androidx.compose.runtime.snapshots.Snapshot
 import androidx.compose.ui.geometry.Offset
 import com.ethran.notable.SCREEN_HEIGHT
 import com.ethran.notable.SCREEN_WIDTH
@@ -98,9 +99,24 @@ class PageDataManager @Inject constructor(
     private val fileToPages = mutableMapOf<String, MutableSet<String>>()
     val invalidateFileFlow = MutableSharedFlow<String>(extraBufferCapacity = 64)
 
-    // needs to be observable by UI, for scroll bars
-    private var pageHigh = mutableStateMapOf<String, Int>()
-    private var pageScroll = mutableStateMapOf<String, Offset>()
+    // Needs to be observable by UI, for scroll bars (read during composition in ScrollIndicator).
+    // These are Compose snapshot states, but they are written from background coroutines
+    // (page loading, scroll/zoom, cache eviction). Writing a snapshot state from a non-composition
+    // thread while the recomposer is applying its own snapshot throws
+    // "Unsupported concurrent change during composition". All mutations must therefore go through
+    // [mutateUiState], which commits them inside a global mutable snapshot so they are applied
+    // atomically and coordinate with composition instead of racing it.
+    private val pageHigh = mutableStateMapOf<String, Int>()
+    private val pageScroll = mutableStateMapOf<String, Offset>()
+
+    /**
+     * Applies [block] (which mutates the UI-observable snapshot maps [pageHigh] / [pageScroll])
+     * inside a global mutable snapshot. This is required because those maps are read during
+     * composition while being written from arbitrary background threads; committing the change as
+     * its own snapshot prevents the concurrent-modification crash in the recomposer.
+     */
+    private inline fun <T> mutateUiState(block: () -> T): T =
+        Snapshot.withMutableSnapshot(block)
 
     // On change, we need to adjust stroke size.
     private var pageZoom = LinkedHashMap<String, Float>()
@@ -471,7 +487,7 @@ class PageDataManager @Inject constructor(
 
     fun getPageHeight(pageId: String): Int? = pageHigh[pageId]
     fun setPageHeight(pageId: String, height: Int) {
-        pageHigh[pageId] = height
+        mutateUiState { pageHigh[pageId] = height }
     }
 
     fun recomputeHeight(pageId: String): Int {
@@ -480,8 +496,9 @@ class PageDataManager @Inject constructor(
                 return SCREEN_HEIGHT
             }
             val maxStrokeBottom = strokes[pageId]!!.maxOf { it.bottom }.plus(50)
-            pageHigh[pageId] = max(maxStrokeBottom.toInt(), SCREEN_HEIGHT)
-            return pageHigh[pageId]!!
+            val newHeight = max(maxStrokeBottom.toInt(), SCREEN_HEIGHT)
+            mutateUiState { pageHigh[pageId] = newHeight }
+            return newHeight
         }
     }
 
@@ -495,14 +512,20 @@ class PageDataManager @Inject constructor(
         }
     }
 
+    /**
+     * Returns the stored scroll for [pageId], or the page's persisted default if none is cached yet.
+     *
+     * This is a pure read: it never mutates [pageScroll]. Previously it used `getOrPut`, which wrote
+     * to the snapshot map even when called from composition (ScrollIndicator), racing the background
+     * writers and triggering the concurrent-modification crash. The default is computed on the fly;
+     * the entry is only materialized when [setPageScroll] is called from a controlled write path.
+     */
     fun getPageScroll(pageId: String): Offset {
-        return pageScroll.getOrPut(pageId) {
-            Offset(0f, pageFromDb?.scroll?.toFloat() ?: 0f)
-        }
+        return pageScroll[pageId] ?: Offset(0f, pageFromDb?.scroll?.toFloat() ?: 0f)
     }
 
     fun setPageScroll(pageId: String, scroll: Offset) {
-        pageScroll[pageId] = scroll
+        mutateUiState { pageScroll[pageId] = scroll }
     }
 
     fun getPageZoom(pageId: String): Float = pageZoom.getOrPut(pageId) { 1f }
@@ -900,9 +923,12 @@ class PageDataManager @Inject constructor(
         synchronized(accessLock) {
             strokes.remove(pageId)
             images.remove(pageId)
-            pageHigh.remove(pageId)
+            // pageHigh/pageScroll are UI-observable snapshot state: remove them in a snapshot.
+            mutateUiState {
+                pageHigh.remove(pageId)
+                pageScroll.remove(pageId)
+            }
             pageZoom.remove(pageId)
-            pageScroll.remove(pageId)
             bitmapCache.remove(pageId)
             strokesById.remove(pageId)
             imagesById.remove(pageId)

app/src/main/java/com/ethran/notable/data/datastore/AppSettings.kt

@@ -1,6 +1,7 @@
 package com.ethran.notable.data.datastore
 
 import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.snapshots.Snapshot
 import kotlinx.serialization.Serializable
 
 
@@ -15,8 +16,17 @@ object GlobalAppSettings {
     val current: AppSettings
         get() = _current.value
 
+    /**
+     * Updates the globally observed settings. This is a Compose snapshot state read all over the UI
+     * during composition, yet [update] is called from background coroutines (e.g. when persisting
+     * settings on Dispatchers.IO). Writing the snapshot state directly from a non-composition thread
+     * can race the recomposer and throw "Unsupported concurrent change during composition", so the
+     * write is committed inside its own global mutable snapshot.
+     */
     fun update(settings: AppSettings) {
-        _current.value = settings
+        Snapshot.withMutableSnapshot {
+            _current.value = settings
+        }
     }
 }
 

app/src/test/java/com/ethran/notable/data/SnapshotStateMapConcurrencyTest.kt

@@ -0,0 +1,145 @@
+package com.ethran.notable.data
+
+import androidx.compose.runtime.mutableStateMapOf
+import androidx.compose.runtime.snapshots.Snapshot
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+import org.junit.Test
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.Executors
+import java.util.concurrent.TimeUnit
+import java.util.concurrent.atomic.AtomicReference
+
+/**
+ * Guards the contract that [PageDataManager] relies on for its UI-observable snapshot maps
+ * (`pageHigh` / `pageScroll`).
+ *
+ * Those maps are read during composition (ScrollIndicator) and written from background coroutines
+ * (page loading, scroll/zoom, cache eviction). Writing a Compose snapshot state from a
+ * non-composition thread while the recomposer applies its own snapshot throws
+ * "Unsupported concurrent change during composition". [PageDataManager] avoids this by funneling
+ * every mutation through `Snapshot.withMutableSnapshot { ... }`.
+ *
+ * These tests pin down that reasoning so the fix can't be silently reverted: snapshot-guarded
+ * writes and removals must interleave with concurrent reads without throwing.
+ */
+class SnapshotStateMapConcurrencyTest {
+
+    @Test
+    fun guardedWrite_isVisibleToLaterReaders() {
+        val map = mutableStateMapOf<String, Int>()
+
+        Snapshot.withMutableSnapshot { map["page"] = 42 }
+
+        assertEquals(42, map["page"])
+    }
+
+    /**
+     * Many threads mutate the same snapshot map through `Snapshot.withMutableSnapshot` while a reader
+     * thread keeps reading entries (mimicking composition reading `page.scroll` / `page.height`).
+     * This must complete without surfacing a concurrent-modification failure.
+     *
+     * Note: Writers are synchronized to avoid [SnapshotApplyConflictException] when multiple
+     * background threads attempt to apply to the global snapshot simultaneously. This still
+     * validates that guarded writes do not crash concurrent readers.
+     */
+    @Test
+    fun guardedConcurrentWrites_doNotThrow() {
+        val map = mutableStateMapOf<String, Int>()
+        val keys = (0 until 16).map { "page-$it" }
+        Snapshot.withMutableSnapshot { keys.forEach { map[it] = 0 } }
+
+        val writers = 8
+        val writesPerThread = 1000
+        val pool = Executors.newFixedThreadPool(writers + 1)
+        val start = CountDownLatch(1)
+        val failure = AtomicReference<Throwable?>(null)
+        val writeLock = Any()
+
+        val readerDone = CountDownLatch(1)
+        pool.submit {
+            try {
+                start.await()
+                repeat(writers * writesPerThread) {
+                    keys.forEach { map[it] }
+                }
+            } catch (t: Throwable) {
+                failure.compareAndSet(null, t)
+            } finally {
+                readerDone.countDown()
+            }
+        }
+
+        val writersDone = CountDownLatch(writers)
+        repeat(writers) { writerIndex ->
+            pool.submit {
+                try {
+                    start.await()
+                    repeat(writesPerThread) { i ->
+                        // Each writer owns its own key, matching per-page writes in PageDataManager.
+                        synchronized(writeLock) {
+                            Snapshot.withMutableSnapshot { map["page-$writerIndex"] = i }
+                        }
+                    }
+                } catch (t: Throwable) {
+                    failure.compareAndSet(null, t)
+                } finally {
+                    writersDone.countDown()
+                }
+            }
+        }
+
+        start.countDown()
+        writersDone.await(30, TimeUnit.SECONDS)
+        readerDone.await(30, TimeUnit.SECONDS)
+        pool.shutdownNow()
+
+        assertNull("Snapshot-guarded writes must not throw", failure.get())
+    }
+
+    /**
+     * Removing entries (cache eviction during page switching) through the guarded path must also be
+     * safe to interleave with concurrent reads.
+     */
+    @Test
+    fun guardedRemovalDuringReads_doesNotThrow() {
+        val map = mutableStateMapOf<String, Int>()
+        val keys = (0 until 200).map { "page-$it" }
+        Snapshot.withMutableSnapshot { keys.forEachIndexed { index, key -> map[key] = index } }
+
+        val pool = Executors.newFixedThreadPool(2)
+        val start = CountDownLatch(1)
+        val failure = AtomicReference<Throwable?>(null)
+        val done = CountDownLatch(2)
+
+        pool.submit {
+            try {
+                start.await()
+                repeat(2000) {
+                    map.values.sum()
+                }
+            } catch (t: Throwable) {
+                failure.compareAndSet(null, t)
+            } finally {
+                done.countDown()
+            }
+        }
+
+        pool.submit {
+            try {
+                start.await()
+                keys.forEach { key -> Snapshot.withMutableSnapshot { map.remove(key) } }
+            } catch (t: Throwable) {
+                failure.compareAndSet(null, t)
+            } finally {
+                done.countDown()
+            }
+        }
+
+        start.countDown()
+        done.await(30, TimeUnit.SECONDS)
+        pool.shutdownNow()
+
+        assertNull("Guarded removal must not throw during reads", failure.get())
+    }
+}

app/src/test/java/com/ethran/notable/data/datastore/GlobalAppSettingsConcurrencyTest.kt

@@ -0,0 +1,93 @@
+package com.ethran.notable.data.datastore
+
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertNull
+import org.junit.Test
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.Executors
+import java.util.concurrent.TimeUnit
+import java.util.concurrent.atomic.AtomicReference
+
+/**
+ * Regression tests for the "Unsupported concurrent change during composition" crash.
+ *
+ * [GlobalAppSettings.current] is a Compose snapshot state that is read all over the UI during
+ * composition, while [GlobalAppSettings.update] is invoked from background coroutines. Updating the
+ * snapshot state directly from a non-composition thread used to race the recomposer and crash.
+ * [GlobalAppSettings.update] now commits inside a global mutable snapshot, which must make
+ * concurrent background writes safe against concurrent reads.
+ */
+class GlobalAppSettingsConcurrencyTest {
+
+    @Test
+    fun update_isVisibleAfterCommit() {
+        GlobalAppSettings.update(AppSettings(version = 1, neoTools = false))
+        GlobalAppSettings.update(AppSettings(version = 1, neoTools = true))
+
+        assertEquals(true, GlobalAppSettings.current.neoTools)
+    }
+
+    /**
+     * Hammers [GlobalAppSettings.update] from many background threads while a separate thread keeps
+     * reading [GlobalAppSettings.current], mimicking composition observing the value. Before the fix
+     * this pattern could surface a concurrent-modification failure; with the snapshot-guarded write
+     * it must complete without throwing.
+     *
+     * Note: Writers are synchronized to avoid [SnapshotApplyConflictException] when multiple
+     * background threads attempt to apply to the global snapshot simultaneously. This still
+     * validates that guarded writes do not crash concurrent readers.
+     */
+    @Test
+    fun concurrentBackgroundUpdates_doNotThrow() {
+        val writers = 8
+        val updatesPerWriter = 500
+        val pool = Executors.newFixedThreadPool(writers + 1)
+        val start = CountDownLatch(1)
+        val failure = AtomicReference<Throwable?>(null)
+        val writeLock = Any()
+
+        val readerDone = CountDownLatch(1)
+        pool.submit {
+            try {
+                start.await()
+                repeat(writers * updatesPerWriter) {
+                    // Plain read, as composition would observe it.
+                    GlobalAppSettings.current.version
+                }
+            } catch (t: Throwable) {
+                failure.compareAndSet(null, t)
+            } finally {
+                readerDone.countDown()
+            }
+        }
+
+        val writersDone = CountDownLatch(writers)
+        repeat(writers) { writerIndex ->
+            pool.submit {
+                try {
+                    start.await()
+                    repeat(updatesPerWriter) { i ->
+                        synchronized(writeLock) {
+                            GlobalAppSettings.update(
+                                AppSettings(version = writerIndex * updatesPerWriter + i)
+                            )
+                        }
+                    }
+                } catch (t: Throwable) {
+                    failure.compareAndSet(null, t)
+                } finally {
+                    writersDone.countDown()
+                }
+            }
+        }
+
+        start.countDown()
+        writersDone.await(30, TimeUnit.SECONDS)
+        readerDone.await(30, TimeUnit.SECONDS)
+        pool.shutdownNow()
+
+        assertNull("Concurrent update must not throw", failure.get())
+        assertNotNull(GlobalAppSettings.current)
+    }
+}