fix(deck-picker): do not block the main thread on startup during sync

[criticalAY]

Note

Assisted-by: Claude Opus 4.8 (some diagnosis and a few parts of code)

Purpose / Description

Fixes an ANR when the DeckPicker is recreated while a sync is stuck on an unresponsive server (for example AnkiWeb being down).

Fixes

• Fixes If ankiweb does not respond, the UI does not respond #21305

Approach

See commit

How Has This Been Tested?

Unit test

Learning (optional, can help others)

NA

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[david-allison]

Could you provide reproduction instructions (probably a patch which adds a delay) so we can reproduce it on a physical device.

The startup path has historically been nasty. This initially looks good (and the ViewModel makes things easier), but I'd like to run it through a debugger to confirm.

[criticalAY]

Subject: [PATCH] repro: sync anr
---
Index: AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt b/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt	(revision 25da95364f62624be98404df64afae4d699f134c)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/Sync.kt	(date 1783413600156)
@@ -159,6 +159,8 @@
             manualCancelButton = R.string.dialog_cancel,
         ) {
             withCol {
+                Timber.w("#21305 repro: holding the collection queue for 60s")
+                Thread.sleep(60_000)
                 syncCollection(auth2, syncMedia = false) // media is synced by SyncMediaWorker
             }
         }
Index: AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt b/AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt	(revision 25da95364f62624be98404df64afae4d699f134c)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/deckpicker/DeckPickerViewModel.kt	(date 1783413571943)
@@ -522,17 +522,11 @@
         }
 
         Timber.d("handleStartup: Continuing after permission granted")
-        viewModelScope.launch {
-            // opening the collection waits on the collection queue, which a sync stuck on an
-            // unresponsive server can hold for a long time. Waiting on the main thread here
-            // froze the DeckPicker when it was recreated
-            val failure =
-                withContext(Dispatchers.IO) {
-                    InitialActivity.getStartupFailureType(environment::initializeAnkiDroidFolder)
-                }
+        run {
+            val failure = InitialActivity.getStartupFailureType(environment::initializeAnkiDroidFolder)
             if (failure != null) {
                 flowOfStartupResponse.value = StartupResponse.FatalError(failure)
-                return@launch
+                return
             }
 
             // successful startup

[david-allison]

One suspected blocker, a few nits on the test

[david-allison]

I believe this no longer blanks the options menu.

We don't refresh the menu here, or in DeckPicker::onStartupResponse

Could you add a test to confirm?

[david-allison]

LGTM, cheers!

[david-allison]

e: file:///D:/a/Anki-Android/Anki-Android/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt:26:23 Unresolved reference 'CollectionHelper'.

e: file:///D:/a/Anki-Android/Anki-Android/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt:34:23 Unresolved reference 'storage'.
> Task :AnkiDroid:compilePlayDebugUnitTestKotlin FAILED
e: file:///D:/a/Anki-Android/Anki-Android/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt:275:13 Unresolved reference 'CollectionHelper'.
e: file:///D:/a/Anki-Android/Anki-Android/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt:275:60 Unresolved reference 'StorageDecision'.
e: file:///D:/a/Anki-Android/Anki-Android/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt:294:13 Unresolved reference 'CollectionHelper'.
e: file:///D:/a/Anki-Android/Anki-Android/AnkiDroid/src/test/java/com/ichi2/anki/deckpicker/DeckPickerViewModelTest.kt:304:47 Type of 'val requiredPermissions: PermissionSet' is not a subtype of overridden property 'val requiredPermissions: StoragePermissionSet' defined in 'com.ichi2.anki.deckpicker.DeckPickerViewModel.AnkiDroidEnvironment'.

[criticalAY]

Ok let me resolve the conflicts/issues and lets get it in

[david-allison]

GPT-generated regression test, needs fixing

Index: AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerStartupTest.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerStartupTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerStartupTest.kt
new file mode 100644
--- /dev/null	(date 1790437094418)
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/DeckPickerStartupTest.kt	(date 1790437094418)
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+package com.ichi2.anki
+
+import android.content.SharedPreferences
+import androidx.core.content.edit
+import androidx.lifecycle.ViewModelProvider
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import com.ichi2.anki.deckpicker.DeckPickerViewModel
+import com.ichi2.anki.deckpicker.DeckPickerViewModel.AnkiDroidEnvironment
+import com.ichi2.anki.testutils.SingleViewModelFactory
+import com.ichi2.testutils.BackupManagerTestUtilities
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.mockito.kotlin.any
+import org.mockito.kotlin.doAnswer
+import org.mockito.kotlin.doCallRealMethod
+import org.mockito.kotlin.spy
+import org.mockito.kotlin.whenever
+import org.robolectric.Robolectric
+import org.robolectric.annotation.Config
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.TimeUnit
+import kotlin.test.assertFalse
+import kotlin.test.assertTrue
+import kotlin.time.Duration.Companion.seconds
+
+@RunWith(AndroidJUnit4::class)
+@Config(qualifiers = "normal")
+class DeckPickerStartupTest : RobolectricTest() {
+    override fun getCollectionStorageMode() = CollectionStorageMode.ON_DISK
+
+    @Test
+    fun `decks load when startup opens the collection after the initial refresh`() =
+        runTest {
+            setIntroductionSlidesShown(true)
+            InitialActivity.setUpgradedToLatestVersion(getPreferences())
+            getPreferences().edit { putBoolean("backupPromptDisabled", true) }
+            BackupManagerTestUtilities.setupSpaceForBackup(targetContext)
+            addBasicNote("cold start", "should appear")
+            CollectionManager.closeCollectionBlocking()
+
+            val releaseStartup = CountDownLatch(1)
+            val viewModel = spy(DeckPickerViewModel())
+            // Hold the real startup check before it opens the collection, without occupying
+            // the collection queue: onResume must be able to observe the closed collection.
+            doAnswer { invocation ->
+                val environment = invocation.getArgument<AnkiDroidEnvironment>(0)
+                doCallRealMethod().whenever(viewModel).handleStartup(any())
+                viewModel.handleStartup(
+                    object : AnkiDroidEnvironment by environment {
+                        override val preferences: SharedPreferences
+                            get() {
+                                check(releaseStartup.await(10.seconds.inWholeMilliseconds, TimeUnit.MILLISECONDS)) {
+                                    "The background startup check was not released"
+                                }
+                                return environment.preferences
+                            }
+                    },
+                )
+            }.whenever(viewModel).handleStartup(any())
+
+            val controller = Robolectric.buildActivity(DeckPicker::class.java)
+            saveControllerForCleanup(controller)
+            val activity = controller.get()
+            ViewModelProvider(activity, SingleViewModelFactory.create(viewModel))[DeckPickerViewModel::class.java]
+            try {
+                controller
+                    .create()
+                    .start()
+                    .resume()
+                    .visible()
+                // Complete a refresh with the collection still closed, as can happen onResume.
+                viewModel.updateDeckList().join()
+                assertFalse(CollectionManager.isOpenUnsafe(), "Startup must not have opened the collection yet")
+
+                releaseStartup.countDown()
+                viewModel.startupJob!!.join()
+                advanceRobolectricLooperUntil {
+                    viewModel.flowOfStartupResponse.value == null
+                }
+                assertTrue(CollectionManager.isOpenUnsafe(), "Startup should have opened the collection")
+
+                advanceRobolectricLooperUntil(
+                    timeout = 5.seconds,
+                    lazyMessage = {
+                        "Successful startup must display the decks even if the initial refresh ran before the collection opened"
+                    },
+                ) {
+                    activity.hasAtLeastOneDeckBeingDisplayed()
+                }
+            } finally {
+                releaseStartup.countDown()
+                viewModel.startupJob?.join()
+                BackupManagerTestUtilities.reset()
+            }
+        }
+}

[criticalAY]

Thanks for the test, the first deck refresh could run before the async startup opened the collection, so Success now refreshes the deck list and rebuilds the menu (the menu had the same gap with an empty collection).

Umm also fixed theCI failure, a test racing the async startup, by making the shared start helper wait for DeckPicker's startup; the trade-off is a second deck load on each DeckPicker creation, which I can avoid if you'd prefer.

[david-allison]

Let's do it right, now the cost of doing it right is so much less

First impressions matter