-
-
Notifications
You must be signed in to change notification settings - Fork 27
fix: contain uncaught coroutine exception in AppLifecycleObserver foreground sync (#566) #569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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) { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catching It is safer and more idiomatic to catch
Suggested change
|
||||||
| Logger.e(e) { "SyncTrigger: onAppForeground failed" } | ||||||
| onSyncFailure(e) | ||||||
| } | ||||||
| attemptSync(bypassThrottle = false) | ||||||
| } | ||||||
|
|
||||||
| private suspend fun syncHealthBodyWeightFromConnectedPlatform() { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the redundant |
||
|
|
||
| /** 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") | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| 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<CancellationException> { | ||
| 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", | ||
| ) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
try-catchblock inAppLifecycleObserveris redundant becausesyncTriggerManager.onAppForeground()is already fully wrapped in its owntry-catchblock that catches all non-cancellation exceptions, logs them, and handles the failure gracefully.Additionally, keeping this outer
try-catchintroduces a maintainability risk: if any other code is added to thislaunchblock in the future, any exceptions thrown by it would be caught here and logged with the misleading message"AppLifecycleObserver: onAppForeground failed".We should simplify this by removing the redundant
try-catchwrapper and directly callingsyncTriggerManager.onAppForeground().