From ca6d90b6029f459d2049b46d15149bc2d6f2b787 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 6 Jul 2026 14:46:40 -0500 Subject: [PATCH 1/2] test(settings): cancel viewModelScope before db.close in SignaturesScreenTest to fix a Room teardown race (flaky on API-37 CI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SignaturesScreenTest built a real SignaturesViewModel by hand but tore down with a bare `db.close()` that never cancelled viewModelScope. The ViewModel's `signatures` StateFlow is a Room InvalidationTracker Flow kept alive by stateIn(WhileSubscribed(5_000)), so the collector could stay live up to 5s after the UI detached — a re-query then landed on the just-closed in-memory DB and threw SQLITE_MISUSE ("connection is closed"). Timing-dependent, hence the intermittent API-37 CI failure in tappingRadioOnNonDefault_makesItTheDefault. Fix: hold the ViewModel in an androidx.lifecycle.ViewModelStore and, in @After, call store.clear() (→ ViewModel.onCleared() → cancels viewModelScope) BEFORE db.close(), so the collector is gone before the DB closes. Behaviour and assertions are unchanged; the fix removes the race by construction. Audited the androidTest tree for the same hazard and fixed two siblings the same way: - AccountSettingsScreenTest: had the same live-Room-Flow-vs-close race, previously worked around by never closing the in-memory DB at all; now clears the ViewModel then closes the DB. - ComposeScreenTest: ComposeViewModel's init launches a viewModelScope coroutine that reads the real accountSettings/signature Room repos; clear the store before db.close() to avoid the same in-flight-read-vs-close race. Verified locally: connectedDebugAndroidTest green for all three classes (12/12) on a cold-booted emulator, plus the JVM fast gate (assembleDebug, testDebugUnitTest, jacocoTestCoverageVerification, compileDebugAndroidTestKotlin, lintDebug, ktlintCheck, detekt). Co-Authored-By: Claude Opus 4.8 --- .../libremail/ui/compose/ComposeScreenTest.kt | 12 +++++++++ .../ui/settings/AccountSettingsScreenTest.kt | 27 +++++++++++++++---- .../ui/settings/SignaturesScreenTest.kt | 16 ++++++++++- 3 files changed, 49 insertions(+), 6 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt index 13e83f4..2180de9 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt @@ -15,6 +15,7 @@ import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performTextInput import androidx.lifecycle.SavedStateHandle +import androidx.lifecycle.ViewModelStore import androidx.room.Room import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry @@ -60,10 +61,20 @@ class ComposeScreenTest { private var db: AccountDatabase? = null + // Holds the real ComposeViewModel built by hand in setContent() below, so closeDb() can clear() + // it (triggering ViewModel.onCleared()) before closing the DB. + private val viewModelStore = ViewModelStore() + private fun string(resId: Int) = composeTestRule.activity.getString(resId) @After fun closeDb() { + // Clear the store (→ ViewModel.onCleared() → cancels viewModelScope) BEFORE closing the DB. + // ComposeViewModel's init block launches a viewModelScope coroutine that reads the real + // accountSettings/signature Room repositories (applySignature()); without this, that read can + // still be in flight when the DB closes, racing a SQLITE_MISUSE ("connection is closed") — + // the same class of teardown race fixed in SignaturesScreenTest/AccountSettingsScreenTest. + viewModelStore.clear() db?.close() } @@ -92,6 +103,7 @@ class ComposeScreenTest { signatureRepository = SignatureRepository(database.signatureDao()), settingsRepository = SettingsRepository(context), ) + viewModelStore.put("compose", viewModel) composeTestRule.setContent { LibreMailTheme(darkTheme = false, dynamicColor = false) { ComposeScreen(onBack = onBack, viewModel = viewModel) diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt index 1bbd7b3..53b61c5 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt @@ -7,11 +7,13 @@ import androidx.compose.ui.test.junit4.createAndroidComposeRule import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.lifecycle.SavedStateHandle +import androidx.lifecycle.ViewModelStore import androidx.room.Room import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.work.WorkManager import kotlinx.coroutines.runBlocking +import org.junit.After import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -54,13 +56,27 @@ class AccountSettingsScreenTest { private var manageSignaturesClicked = false + private lateinit var db: AccountDatabase + + // Holds the real AccountSettingsViewModel built by hand in setContent() below, so tearDown() can + // clear() it (triggering ViewModel.onCleared()) before closing the DB. + private val viewModelStore = ViewModelStore() + + @After + fun tearDown() { + // Clear the store (→ ViewModel.onCleared() → cancels viewModelScope) BEFORE closing the DB. + // The ViewModel's `settings`/`signatureCount`/`defaultSignatureName`/`account` Room + // InvalidationTracker Flows are kept alive by stateIn(WhileSubscribed(5_000)): without this, + // a collector can still be live up to 5s after the UI detaches, so a re-query lands on the + // just-closed in-memory DB and throws SQLITE_MISUSE ("connection is closed") — an intermittent + // teardown race, not a real bug. (Previously worked around by never closing the DB at all.) + viewModelStore.clear() + db.close() + } + private fun setContent(): AccountSettingsRepository { val context = ApplicationProvider.getApplicationContext() - // Intentionally not closed in an @After: the ViewModel's `settings` Room Flow (kept alive by - // stateIn/WhileSubscribed) keeps querying after the test body, so closing the in-memory DB out - // from under it races and crashes ("connection pool has been closed"). The DB is reclaimed with - // the test process. - val db = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build() + db = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build() val repository = AccountSettingsRepository(db.accountSettingsDao()) runBlocking { db.accountDao().upsert(account.toEntity()) // FK parent for the account_settings row @@ -74,6 +90,7 @@ class AccountSettingsScreenTest { syncScheduler = SyncScheduler(Provider { WorkManager.getInstance(context) }), settingsRepository = SettingsRepository(context), ) + viewModelStore.put("account-settings", viewModel) composeTestRule.setContent { LibreMailTheme(darkTheme = false, dynamicColor = false) { AccountSettingsScreen( diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/SignaturesScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/SignaturesScreenTest.kt index 0a28ae2..a75cc8c 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/SignaturesScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/SignaturesScreenTest.kt @@ -14,6 +14,7 @@ import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.lifecycle.SavedStateHandle +import androidx.lifecycle.ViewModelStore import androidx.room.Room import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry @@ -50,6 +51,10 @@ class SignaturesScreenTest { private lateinit var db: AccountDatabase private lateinit var repository: SignatureRepository + // Holds the real SignaturesViewModel built by hand in setContent() below, so tearDown() can + // clear() it (triggering ViewModel.onCleared()) before closing the DB. + private val viewModelStore = ViewModelStore() + @Before fun setUp() { db = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build() @@ -70,7 +75,15 @@ class SignaturesScreenTest { } @After - fun tearDown() = db.close() + fun tearDown() { + // Clear the store (→ ViewModel.onCleared() → cancels viewModelScope) BEFORE closing the DB. + // SignaturesViewModel.signatures is a Room InvalidationTracker Flow kept alive by + // stateIn(WhileSubscribed(5_000)): without this, the collector can still be live up to 5s + // after the UI detaches, so a re-query lands on the just-closed in-memory DB and throws + // SQLITE_MISUSE ("connection is closed") — an intermittent teardown race, not a real bug. + viewModelStore.clear() + db.close() + } private fun string(resId: Int) = composeTestRule.activity.getString(resId) @@ -81,6 +94,7 @@ class SignaturesScreenTest { SavedStateHandle(mapOf(Routes.SIGNATURES_ARG_ACCOUNT to accountId)), repository, ) + viewModelStore.put("signatures", viewModel) composeTestRule.setContent { LibreMailTheme(darkTheme = false, dynamicColor = false) { SignaturesScreen(onBack = {}, onEdit = {}, onAdd = {}, viewModel = viewModel) From 187a8effb0f961b531b45495d9418bb8f165ffac Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Mon, 6 Jul 2026 15:53:54 -0500 Subject: [PATCH 2/2] ci(e2e): capture logcat + emulator/system diagnostics across the E2E matrix (#387) The API 29-36 `e2e` matrix uploaded only its test report, so an emulator flake or a red leg (e.g. `E2E (31)` dying on a bare `sdkmanager` exit 1) left nothing to diagnose. Bring the #334 API-37 diagnostics to the matrix, inline (no changes to `e2e-preview`, which PR #372 is restructuring): - Stream `adb logcat -v time` to `$RUNNER_TEMP/logcat-api.txt` at the top of both the "Run E2E tests" and retry reactivecircus steps (emulator is booted there); backgrounded so gradle stays the exit-status-bearing command. - New `if: failure()` step dumps device + runner state (adb devices, logcat tail, emulator -accel-check, /dev/kvm, free -h, df -h) to the step log and a diagnostics file; every probe guarded with `|| true`. - New `if: always()` upload-artifact (same pinned v7 SHA) `e2e-diagnostics-api` carries the logcat + diagnostics files, `if-no-files-found: warn`. - Make "Install SDK platform and build-tools" diagnosable: bounded 3x retry with backoff for a transient sdkmanager failure, and print `--list_installed` on a hard failure instead of a bare exit 1. Keeps reactivecircus/android-emulator-runner and the existing boot-race retry. Additive/diagnostic only; no boot-affecting flags change. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/ci.yml | 67 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4c0b51a..7c22280 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -228,8 +228,25 @@ jobs: - name: Set up Android SDK uses: android-actions/setup-android@40fd30fb8d7440372e1316f5d1809ec01dcd3699 # v4.0.1 + # sdkmanager can exit 1 on a transient package-mirror/network hiccup with no useful trail — + # an `E2E (31)` leg died exactly this way (#387). Retry up to 3x with backoff so a transient + # failure self-heals, and on a hard failure print the installed-package list so the cause is + # visible in the step log instead of a bare exit 1. - name: Install SDK platform and build-tools - run: sdkmanager "$ANDROID_PLATFORM" "$ANDROID_BUILD_TOOLS" + run: | + for attempt in 1 2 3; do + echo "::group::sdkmanager install (attempt $attempt)" + if sdkmanager "$ANDROID_PLATFORM" "$ANDROID_BUILD_TOOLS"; then + echo "::endgroup::" + exit 0 + fi + echo "::endgroup::" + echo "::warning::sdkmanager attempt $attempt failed to install $ANDROID_PLATFORM / $ANDROID_BUILD_TOOLS" + if [ "$attempt" -lt 3 ]; then sleep "$((attempt * 15))"; fi + done + echo "::error::sdkmanager failed to install the SDK packages after 3 attempts" + echo "--- sdkmanager --list_installed ---"; sdkmanager --list_installed 2>&1 || true + exit 1 - name: Set up Gradle uses: gradle/actions/setup-gradle@3f131e8634966bd73d06cc69884922b02e6faf92 # v6.2.0 @@ -281,7 +298,13 @@ jobs: force-avd-creation: false emulator-options: -no-snapshot-save -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none disable-animations: true - script: ./gradlew connectedDebugAndroidTest --stacktrace + # Stream logcat to a per-api-level file (the emulator is booted here) before the tests, + # so a test failure or emulator flake is diagnosable from the uploaded artifact. + # Backgrounded; gradle stays the last foreground command so the step's exit status is + # still the test result (a real failure still trips continue-on-error -> the retry). + script: | + adb logcat -v time > "$RUNNER_TEMP/logcat-api${{ matrix.api-level }}.txt" 2>&1 & + ./gradlew connectedDebugAndroidTest --stacktrace - name: Run E2E tests (retry after emulator boot race) if: steps.e2e.outcome == 'failure' @@ -293,7 +316,31 @@ jobs: force-avd-creation: false emulator-options: -no-snapshot-save -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none disable-animations: true - script: ./gradlew connectedDebugAndroidTest --stacktrace + # Retry runs a fresh emulator boot; stream its logcat the same way. `>` overwrites + # attempt 1's file so the artifact holds the FINAL attempt's logs, matching the + # failure-time dump below (which reflects this last attempt's state). + script: | + adb logcat -v time > "$RUNNER_TEMP/logcat-api${{ matrix.api-level }}.txt" 2>&1 & + ./gradlew connectedDebugAndroidTest --stacktrace + + # On any E2E failure (both boot attempts failed, a hung emulator, or an earlier setup/SDK + # step), snapshot device + runner state to the step log AND a file for the artifact upload — + # the `E2E (31)` sdkmanager death (#387) left no trail. Each probe is guarded (|| true) so a + # missing tool / offline device can't abort the step; accel-check, /dev/kvm, free -h and + # df -h characterise the runner even when the emulator never booted. + - name: Dump emulator + system diagnostics on failure + if: failure() + run: | + DIAG="${RUNNER_TEMP:-/tmp}/diagnostics-api${{ matrix.api-level }}.txt" + { + echo "===== E2E API ${{ matrix.api-level }} failure diagnostics =====" + echo "--- adb devices ---"; adb devices 2>&1 || true + echo "--- adb logcat -d (tail 200) ---"; adb logcat -d 2>&1 | tail -200 || true + echo "--- emulator -accel-check ---"; "$ANDROID_SDK_ROOT/emulator/emulator" -accel-check 2>&1 || true + echo "--- /dev/kvm ---"; ls -l /dev/kvm 2>&1 || true + echo "--- free memory ---"; free -h 2>&1 || true + echo "--- free disk ---"; df -h 2>&1 || true + } 2>&1 | tee "$DIAG" - name: Upload E2E test report if: ${{ !cancelled() }} @@ -303,6 +350,20 @@ jobs: path: app/build/reports/androidTests/connected/ if-no-files-found: warn + # Always upload the streamed logcat + (on failure) the system-state dump so an emulator flake + # or a red matrix leg is diagnosable from artifacts without a re-run — parity with the + # e2e-preview boot-diagnostics artifact. Per-api-level name (upload-artifact@v7 rejects + # duplicate artifact names). + - name: Upload E2E diagnostics + if: always() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: e2e-diagnostics-api${{ matrix.api-level }} + path: | + ${{ runner.temp }}/logcat-api${{ matrix.api-level }}.txt + ${{ runner.temp }}/diagnostics-api${{ matrix.api-level }}.txt + if-no-files-found: warn + # API 37 (Android 17, preview) E2E. Its only system image is the nonstandard # android-37.0 / google_apis_ps16k (16 KB page size), which reactivecircus/android-emulator-runner # can't provision (it builds android-37 / google_apis, neither of which exists), so this job