diff --git a/shared/src/commonMain/kotlin/com/devil/phoenixproject/App.kt b/shared/src/commonMain/kotlin/com/devil/phoenixproject/App.kt index 8dbbfa2cf..881a16a42 100644 --- a/shared/src/commonMain/kotlin/com/devil/phoenixproject/App.kt +++ b/shared/src/commonMain/kotlin/com/devil/phoenixproject/App.kt @@ -29,6 +29,7 @@ import androidx.compose.ui.unit.sp import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleEventObserver import androidx.lifecycle.compose.LocalLifecycleOwner +import co.touchlab.kermit.Logger import com.devil.phoenixproject.data.repository.ExerciseRepository import com.devil.phoenixproject.data.sync.SyncTriggerManager import com.devil.phoenixproject.presentation.screen.EnhancedMainScreen @@ -40,6 +41,7 @@ import com.devil.phoenixproject.presentation.viewmodel.ThemeViewModel import com.devil.phoenixproject.ui.theme.VitruvianTheme import com.devil.phoenixproject.ui.theme.isDynamicColorAvailable import kotlin.time.Clock +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.delay import kotlinx.coroutines.launch @@ -57,7 +59,13 @@ private fun AppLifecycleObserver(syncTriggerManager: SyncTriggerManager) { val observer = LifecycleEventObserver { _, event -> if (event == Lifecycle.Event.ON_RESUME) { scope.launch { - syncTriggerManager.onAppForeground() + try { + syncTriggerManager.onAppForeground() + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + Logger.e(e) { "AppLifecycleObserver: onAppForeground failed" } + } } } } diff --git a/shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/SyncTriggerManager.kt b/shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/SyncTriggerManager.kt index a1dade7c1..2a4a65160 100644 --- a/shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/SyncTriggerManager.kt +++ b/shared/src/commonMain/kotlin/com/devil/phoenixproject/data/sync/SyncTriggerManager.kt @@ -5,6 +5,7 @@ import com.devil.phoenixproject.data.integration.HealthBodyWeightSyncManager import com.devil.phoenixproject.util.ConnectivityChecker import com.devil.phoenixproject.util.withPlatformLock import kotlin.time.Clock +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow @@ -89,12 +90,19 @@ class SyncTriggerManager( * Respects throttle/backoff to avoid excessive sync attempts. */ suspend fun onAppForeground() { - Logger.d { "SyncTrigger: App foreground, checking if sync needed" } - syncHealthBodyWeightFromConnectedPlatform() - if (syncManager.isAuthenticated.value) { - syncManager.refreshPremiumStatusFromServer() + try { + Logger.d { "SyncTrigger: App foreground, checking if sync needed" } + syncHealthBodyWeightFromConnectedPlatform() + if (syncManager.isAuthenticated.value) { + syncManager.refreshPremiumStatusFromServer() + } + attemptSync(bypassThrottle = false) + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + Logger.e(e) { "SyncTrigger: onAppForeground failed" } + onSyncFailure(e) } - attemptSync(bypassThrottle = false) } private suspend fun syncHealthBodyWeightFromConnectedPlatform() { diff --git a/shared/src/commonTest/kotlin/com/devil/phoenixproject/AppLifecycleCoroutineContainmentTest.kt b/shared/src/commonTest/kotlin/com/devil/phoenixproject/AppLifecycleCoroutineContainmentTest.kt new file mode 100644 index 000000000..40b4867af --- /dev/null +++ b/shared/src/commonTest/kotlin/com/devil/phoenixproject/AppLifecycleCoroutineContainmentTest.kt @@ -0,0 +1,90 @@ +package com.devil.phoenixproject + +import kotlin.test.Test +import kotlin.test.assertFalse +import kotlin.test.assertNull +import kotlin.test.assertTrue +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineExceptionHandler +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runTest + +/** + * Regression test for issue #566: uncaught Kotlin coroutine exception from + * AppLifecycleObserver aborts the process on foreground (TestFlight 0.9.1 + * SIGABRT after wake on iOS-on-mac). + * + * AppLifecycleObserver (App.kt) launches syncTriggerManager.onAppForeground() + * inside scope.launch { ... } using rememberCoroutineScope() — a plain Job with + * no SupervisorJob and no CoroutineExceptionHandler. Before the fix the launch + * body had no try/catch, so any non-CancellationException throwable from the + * foreground sync chain reached propagateExceptionFinalResort -> + * processUnhandledException -> terminateWithUnhandledException -> abort(). + * + * These tests pin the containment pattern the fix introduced in App.kt: + * - a non-CancellationException throwable from onAppForeground() (or any + * transitive call) is caught and logged and does NOT reach the scope's + * CoroutineExceptionHandler — i.e. it would not abort the process; + * - CancellationException is rethrown so coroutine cancellation semantics are + * preserved (the launched job is cancelled, the handler is still not invoked). + * + * The scope is constructed as a standalone root (plain Job + + * CoroutineExceptionHandler + Dispatchers.Unconfined) to mirror + * rememberCoroutineScope()'s plain Job and to make CoroutineExceptionHandler + * observable for root coroutines (handlers are ignored for child coroutines). + */ +class AppLifecycleCoroutineContainmentTest { + + /** A non-CancellationException throwable, standing in for a Ktor/IO failure after wake. */ + private class SimulatedForegroundCrash(message: String) : Exception(message) + + /** Mimics rememberCoroutineScope(): plain Job, no SupervisorJob, plus an observable handler. */ + private fun newLifecycleScope(onUncaught: (Throwable) -> Unit): CoroutineScope { + val handler = CoroutineExceptionHandler { _, throwable -> onUncaught(throwable) } + return CoroutineScope(Job() + Dispatchers.Unconfined + handler) + } + + /** The exact wrapper AppLifecycleObserver applies around onAppForeground(). */ + private suspend fun CoroutineScope.launchForegroundGuarded(body: suspend () -> Unit): Job = + launch { + try { + body() + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + // Logger.e(e) { "AppLifecycleObserver: onAppForeground failed" } — swallowed, not propagated. + } + } + + @Test + fun nonCancellationThrowableDoesNotReachUncaughtHandler() = runTest { + var uncaught: Throwable? = null + val scope = newLifecycleScope { uncaught = it } + + val job = scope.launchForegroundGuarded { + throw SimulatedForegroundCrash("simulated network failure after wake") + } + job.join() + + assertTrue(job.isCompleted, "foreground launch should complete after a swallowed throwable") + assertFalse(job.isCancelled, "a swallowed non-Cancellation throwable must not cancel the job") + assertNull(uncaught, "non-Cancellation throwable must not reach CoroutineExceptionHandler (would abort process)") + } + + @Test + fun cancellationExceptionIsRethrownAndCancelsJob() = runTest { + var uncaught: Throwable? = null + val scope = newLifecycleScope { uncaught = it } + + val job = scope.launchForegroundGuarded { + throw CancellationException("lifecycle cancelled") + } + job.join() + + assertTrue(job.isCancelled, "CancellationException must be rethrown so the job is cancelled") + assertNull(uncaught, "CancellationException must not be reported as an uncaught exception") + } +} diff --git a/shared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/SyncTriggerManagerTest.kt b/shared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/SyncTriggerManagerTest.kt index fb5379f65..20f22e296 100644 --- a/shared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/SyncTriggerManagerTest.kt +++ b/shared/src/commonTest/kotlin/com/devil/phoenixproject/data/sync/SyncTriggerManagerTest.kt @@ -3,9 +3,12 @@ package com.devil.phoenixproject.data.sync import com.devil.phoenixproject.domain.model.currentTimeMillis import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse +import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertTrue +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.test.runTest @@ -76,6 +79,17 @@ class SyncTriggerManagerTest { fun setLastSyncTime(time: Long) { _lastSyncTime.value = time } + + // Mimics SyncManager.refreshPremiumStatusFromServer() — a suspend API call + // that can throw raw (Ktor/IO) and is NOT wrapped in Result. This is the + // real crash path for issue #566 when the network stack fails after wake. + var refreshPremiumStatusThrows: Throwable? = null + var refreshPremiumCallCount = 0 + + suspend fun refreshPremiumStatusFromServer() { + refreshPremiumCallCount++ + refreshPremiumStatusThrows?.let { throw it } + } } /** @@ -120,10 +134,24 @@ class SyncTriggerManagerTest { } /** - * Simulates onAppForeground - respects throttle + * Simulates onAppForeground - respects throttle. + * + * Mirrors the issue #566 fix: the body is wrapped in try/catch so a raw + * throwable from refreshPremiumStatusFromServer() (the real crash path) + * is recorded via onSyncFailure() and does NOT propagate, while + * CancellationException is rethrown to preserve cancellation semantics. */ suspend fun onAppForeground() { - attemptSync(bypassThrottle = false) + try { + if (testSyncManager.isAuthenticated.value) { + testSyncManager.refreshPremiumStatusFromServer() + } + attemptSync(bypassThrottle = false) + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + onSyncFailure(e) + } } /** @@ -675,4 +703,56 @@ class SyncTriggerManagerTest { "Single rate limit should not trigger persistent error", ) } + + // ==================== Issue #566 Foreground Crash Containment Tests ==================== + + /** + * Issue #566: a raw throwable from refreshPremiumStatusFromServer() (the real + * crash path — a suspend API call not wrapped in Result) must be recorded in + * RetryState via onSyncFailure() and must NOT propagate out of onAppForeground(). + */ + @Test + fun onAppForegroundRecordsFailureAndDoesNotPropagateWhenPremiumRefreshThrows() = runTest { + val syncManager = TestSyncManager() + val connectivity = TestConnectivityChecker() + val triggerManager = TestableSyncTriggerManager(syncManager, connectivity) + + class SimulatedPremiumRefreshCrash(message: String) : Exception(message) + syncManager.refreshPremiumStatusThrows = + SimulatedPremiumRefreshCrash("simulated premium refresh failure after wake") + + // Must not throw — the throwable is contained by onAppForeground()'s try/catch. + triggerManager.onAppForeground() + + assertEquals(1, syncManager.refreshPremiumCallCount, "Premium refresh should have been attempted") + assertEquals(0, syncManager.syncCallCount, "attemptSync should not be reached after premium refresh threw") + assertEquals(1, triggerManager.getConsecutiveFailures(), "Foreground failure should be recorded") + assertEquals(1, triggerManager.retryState.value.retryCount, "RetryState should reflect the foreground failure") + assertNotNull(triggerManager.getLastErrorCategory(), "Error category should be classified and recorded") + } + + /** + * Issue #566: CancellationException must be rethrown by onAppForeground() so + * coroutine cancellation semantics are preserved, and must NOT be recorded as + * a sync failure. + */ + @Test + fun onAppForegroundRethrowsCancellationException() = runTest { + val syncManager = TestSyncManager() + val connectivity = TestConnectivityChecker() + val triggerManager = TestableSyncTriggerManager(syncManager, connectivity) + + syncManager.refreshPremiumStatusThrows = CancellationException("lifecycle cancelled") + + assertFailsWith { + triggerManager.onAppForeground() + } + + assertEquals(1, syncManager.refreshPremiumCallCount, "Premium refresh should have been attempted") + assertEquals( + 0, + triggerManager.getConsecutiveFailures(), + "CancellationException must not be recorded as a sync failure", + ) + } }