From 52503c000c149b5e2a11ddd4f080570aed0f69d3 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sun, 5 Jul 2026 19:17:19 -0500 Subject: [PATCH] fix(push): stop IdleService dataSync FGS crash-loop on exhausted 24h cap Root cause: after #302's runtime-cap fallBackToPeriodicSync() stops the dataSync foreground service, IdleService was restarted (START_STICKY null-intent redelivery + explicit startForegroundService) and onStartCommand unconditionally called startForeground(DATA_SYNC) while the rolling-24h budget was still exhausted. The platform rejected the start with ForegroundServiceStartNotAllowedException; it was uncaught, the process crashed, and START_STICKY restarted straight back into the same rejection -- a crash loop until the 24h window freed budget (#354). Fix (IdleService.kt): - onStartCommand now returns START_NOT_STICKY. Push is app-managed (LibreMailApplication.ensurePushStarted deterministically restarts it), so the sticky null-intent auto-restart was redundant and fired exactly when a dataSync FGS start is illegal. - Guard the foreground start via a new JVM-testable IdleForegroundStarter seam: a ForegroundServiceStartNotAllowedException (caught via its IllegalStateException supertype, so no minSdk-29 class load) degrades like the cap handler -- schedulePeriodicSync(), keep the degraded POLLING notification, stopSelf() promptly (avoids the "did not call startForeground in time" ANR) -- instead of propagating. - Record the cap event (elapsedRealtime); while still inside the cap window, onStartCommand skips the now-guaranteed-illegal foreground start entirely. - onTimeout stop path kept fast so ForegroundServiceDidNotStopInTimeException stays mitigated. PII-free AppLog.w/i on the degrade paths. Tests: - Unit (IdleForegroundStarterTest): onStartCommand returns START_NOT_STICKY; a rejected start is caught and routed to degrade without propagating; the cap window skips the attempt; a non-ISE propagates. - Instrumented (IdleServiceForegroundStartInstrumentedTest): the degrade path on a real Context -- rejection caught, periodic-sync fallback scheduled, degraded "instant delivery paused" notification built, watching skipped. Co-Authored-By: Claude Opus 4.8 --- ...eServiceForegroundStartInstrumentedTest.kt | 87 ++++++++++++ .../libremail/push/IdleForegroundStarter.kt | 65 +++++++++ .../kotlin/org/libremail/push/IdleService.kt | 125 ++++++++++++++---- .../push/IdleForegroundStarterTest.kt | 104 +++++++++++++++ 4 files changed, 358 insertions(+), 23 deletions(-) create mode 100644 app/src/androidTest/kotlin/org/libremail/push/IdleServiceForegroundStartInstrumentedTest.kt create mode 100644 app/src/main/kotlin/org/libremail/push/IdleForegroundStarter.kt create mode 100644 app/src/test/kotlin/org/libremail/push/IdleForegroundStarterTest.kt diff --git a/app/src/androidTest/kotlin/org/libremail/push/IdleServiceForegroundStartInstrumentedTest.kt b/app/src/androidTest/kotlin/org/libremail/push/IdleServiceForegroundStartInstrumentedTest.kt new file mode 100644 index 0000000..447ce4f --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/push/IdleServiceForegroundStartInstrumentedTest.kt @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.push + +import android.app.Notification +import android.app.Service +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.R +import org.libremail.data.sync.PushMode + +/** + * On-device coverage of the #354 dataSync-FGS degrade path that [IdleService.onStartCommand] routes + * through [IdleForegroundStarter]. When a foreground start is rejected — the runtime-cap + * `ForegroundServiceStartNotAllowedException`, surfaced as its [IllegalStateException] supertype — the + * seam must catch it, skip IDLE watching, and degrade to periodic sync plus the degraded + * ("instant delivery paused") notification, never propagating. This drives the same decision seam the + * service uses and builds the real degraded notification with a real application `Context` (a + * `ContextWrapper`, never a mocked `Context`), mirroring `PushStatusNotificationInstrumentedTest`; it + * stands up no foreground service, Hilt graph, or network, so it is deterministic — and unlike a JVM + * unit test it exercises the real `Notification` build (the unit-test `android.jar`'s + * `NotificationCompat` is a no-op stub). + */ +@RunWith(AndroidJUnit4::class) +class IdleServiceForegroundStartInstrumentedTest { + + private val context = ApplicationProvider.getApplicationContext() + + @Test + fun rejectedForegroundStart_degradesToPeriodicSyncWithPausedNotification_andSkipsWatching() { + val rejection = IllegalStateException( + "Time limit already exhausted for foreground service type dataSync", + ) + var watchingStarted = false + var periodicSyncScheduled = false + var degradedNotification: Notification? = null + + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = false, + enterForeground = { throw rejection }, + onStarted = { watchingStarted = true }, + onDegraded = { cause -> + assertSame("the runtime-cap rejection must reach the degrade path", rejection, cause) + // Mirror IdleService.degradeToPeriodicSync on a real Context: (re)assert periodic sync and + // build the degraded status notification the service would post. + periodicSyncScheduled = true + PushStatusNotification.ensureChannel(context) + degradedNotification = PushStatusNotification.build(context, PushMode.POLLING, timedOut = true) + }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertFalse("a rejected dataSync FGS start must not begin IDLE watching", watchingStarted) + assertTrue("the degrade path must (re)assert the 15-minute periodic sync fallback", periodicSyncScheduled) + val notification = requireNotNull(degradedNotification) { "the degrade path must build a status notification" } + assertEquals( + "the degraded notification must show the instant-delivery-paused text", + context.getString(R.string.notif_push_status_text_timed_out), + notification.extras.getCharSequence(Notification.EXTRA_TEXT).toString(), + ) + } + + @Test + fun activeCapWindow_skipsForegroundStartAttempt_andStillDegrades() { + var enterForegroundAttempted = false + var watchingStarted = false + var degraded = false + + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = true, + enterForeground = { enterForegroundAttempted = true }, + onStarted = { watchingStarted = true }, + onDegraded = { degraded = true }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertFalse("must not attempt a dataSync FGS start while still inside the cap window", enterForegroundAttempted) + assertFalse(watchingStarted) + assertTrue("must fall back to periodic sync while capped", degraded) + } +} diff --git a/app/src/main/kotlin/org/libremail/push/IdleForegroundStarter.kt b/app/src/main/kotlin/org/libremail/push/IdleForegroundStarter.kt new file mode 100644 index 0000000..6dde100 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/push/IdleForegroundStarter.kt @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.push + +import android.app.Service + +/** + * The dataSync foreground-start decision for [IdleService.onStartCommand] (#354), pulled out of the + * Android [Service] so it is JVM-unit-testable without Robolectric (which this repo does not use). + * + * After the Android 14+ dataSync FGS runtime cap fires (#302) and [IdleService] self-stops, the + * service is (re)started — the app deterministically restarts it via + * `LibreMailApplication.ensurePushStarted()`, and previously the platform also auto-restarted it via + * `START_STICKY` with a null intent. Each restart re-entered `onStartCommand` and unconditionally + * called `startForeground(..., dataSync)` while the rolling-24h budget was still exhausted, so the + * platform rejected it with `ForegroundServiceStartNotAllowedException` (uncaught → crash → sticky + * restart → loop). This seam encodes the fix: skip the start outright while still inside the cap + * window, otherwise attempt it and route the rejection — a `ForegroundServiceStartNotAllowedException` + * (API 31+), caught here via its [IllegalStateException] supertype so no `minSdk`-29 class load is + * needed — into a clean degrade instead of letting it propagate. Any other throwable propagates. + */ +internal object IdleForegroundStarter { + + /** + * Enters dataSync foreground state, or degrades to the 15-minute periodic-sync fallback when that + * start is — or would be — illegal. + * + * @param capActive true while still within the runtime-cap window recorded at the last cap event; + * the start is then skipped without attempting it (and without a cause). + * @param enterForeground the real `ServiceCompat.startForeground(..., dataSync)` call; may throw + * `ForegroundServiceStartNotAllowedException` (an [IllegalStateException]) when the runtime cap is + * exhausted or the start raced into the background. + * @param onStarted run after a successful foreground start (proceed with IDLE watchers). + * @param onDegraded run when the start was skipped ([capActive]) or rejected; receives the + * rejection cause, or `null` for the cap-window skip. Must schedule periodic sync and stop the + * service (it was started via `startForegroundService`, so it must stop promptly to avoid the + * "did not call startForeground in time" ANR). + * @return the value `onStartCommand` should return — always [Service.START_NOT_STICKY]: push is + * app-managed, so the platform's sticky null-intent auto-restart is redundant and fires exactly + * in the states that cannot legally start a dataSync FGS. + */ + fun startForegroundOrDegrade( + capActive: Boolean, + enterForeground: () -> Unit, + onStarted: () -> Unit, + onDegraded: (cause: Throwable?) -> Unit, + ): Int { + when { + capActive -> onDegraded(null) + else -> { + val entered = try { + enterForeground() + true + } catch (rejected: IllegalStateException) { + // ForegroundServiceStartNotAllowedException (API 31+) extends IllegalStateException; + // catching the supertype funnels the runtime-cap/background rejection into the + // degrade path without a version gate, instead of crash-looping (#354). + onDegraded(rejected) + false + } + if (entered) onStarted() + } + } + return Service.START_NOT_STICKY + } +} diff --git a/app/src/main/kotlin/org/libremail/push/IdleService.kt b/app/src/main/kotlin/org/libremail/push/IdleService.kt index f41e1ef..2ce39d3 100644 --- a/app/src/main/kotlin/org/libremail/push/IdleService.kt +++ b/app/src/main/kotlin/org/libremail/push/IdleService.kt @@ -8,6 +8,7 @@ import android.content.Intent import android.content.pm.PackageManager import android.content.pm.ServiceInfo import android.os.IBinder +import android.os.SystemClock import androidx.core.app.NotificationManagerCompat import androidx.core.app.ServiceCompat import androidx.core.content.ContextCompat @@ -83,22 +84,54 @@ class IdleService : Service() { /** Active IDLE watcher per account id, so we can start/stop them as accounts change. */ private val watchers = mutableMapOf() - override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { - startAsForeground(shownMode) - if (!watching) { - watching = true - scope.launch { - // Can't open the encrypted DB without the user present. Defer (stop) and let the app - // restart push after the next unlock, rather than block the service and ANR. - if (cacheGuard.isCacheLocked()) { - AppLog.i(TAG, "encrypted cache locked; deferring IDLE push until the app is unlocked") - stopSelf() - return@launch - } - reconcileWatchers() + override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int = + // Always START_NOT_STICKY (never START_STICKY): push is app-managed — LibreMailApplication's + // settings/account collector and ensurePushStarted() deterministically (re)start the service + // whenever it should run — so the platform's sticky null-intent auto-restart is redundant AND, + // after the dataSync FGS runtime cap (#302), re-enters here exactly when a dataSync foreground + // start is illegal, which was the #354 crash loop. The start/degrade decision lives in the + // JVM-testable IdleForegroundStarter seam. + IdleForegroundStarter.startForegroundOrDegrade( + capActive = capWindowActive(), + enterForeground = { startAsForeground(shownMode) }, + onStarted = ::startWatchingIfNeeded, + onDegraded = ::degradeAfterBlockedForegroundStart, + ) + + /** After a successful foreground start, begin watching accounts for IDLE (once per service life). */ + private fun startWatchingIfNeeded() { + if (watching) return + watching = true + scope.launch { + // Can't open the encrypted DB without the user present. Defer (stop) and let the app + // restart push after the next unlock, rather than block the service and ANR. + if (cacheGuard.isCacheLocked()) { + AppLog.i(TAG, "encrypted cache locked; deferring IDLE push until the app is unlocked") + stopSelf() + return@launch } + reconcileWatchers() } - return START_STICKY + } + + /** + * A dataSync foreground start was skipped (still inside the runtime-cap window, [cause] null) or + * rejected by the platform ([cause] is the `ForegroundServiceStartNotAllowedException`). Either way + * degrade like the cap handler instead of crashing (#354): log PII-free and fall back to periodic + * sync. On an actual rejection, also (re)arm the cap window so the next restart skips the attempt. + */ + private fun degradeAfterBlockedForegroundStart(cause: Throwable?) { + if (cause == null) { + AppLog.i(TAG, "dataSync FGS cap still active; skipping foreground start, periodic sync covers mail") + } else { + AppLog.w( + TAG, + "dataSync FGS start rejected (runtime cap or background); staying on 15-minute periodic sync", + cause, + ) + markCapReached() + } + degradeToPeriodicSync() } /** @@ -158,18 +191,33 @@ class IdleService : Service() { } /** - * Clean shutdown for the dataSync FGS runtime-cap timeout (issue #302): re-assert the periodic - * sync fallback, swap the persistent notification to the degraded text and DETACH it so it stays - * posted after we leave foreground state, then stop the service. Stopping foreground state is not - * optional here — a `dataSync` service that is still foreground when its timeout elapses is the - * exact condition the platform force-stops (and throws) on, so we must not keep running as an FGS. - * [stopSelf] then tears down [scope] in [onDestroy], closing the IDLE connections; mail arrives via - * the 15-minute periodic sync until push is started again (next app foreground / cap reset). + * The dataSync FGS runtime-cap timeout (issue #302): record the cap event so the next (re)start + * skips its now-illegal foreground start (#354), then degrade to the periodic-sync fallback. Kept + * fast/synchronous so a `dataSync` service that is still foreground when its timeout elapses — the + * exact condition the platform force-stops (and throws `ForegroundServiceDidNotStopInTimeException`) + * on — leaves foreground state within the grace window. + */ + private fun fallBackToPeriodicSync() { + AppLog.i(TAG, "dataSync FGS runtime cap reached: pausing IMAP IDLE; mail arrives via 15-minute periodic sync") + markCapReached() + degradeToPeriodicSync() + } + + /** + * Shared degrade to the 15-minute periodic-sync fallback, used by the runtime-cap timeout + * ([fallBackToPeriodicSync]) and by [onStartCommand] when a dataSync foreground start is skipped or + * rejected (#354): re-assert the periodic sync, swap the persistent notification to the degraded + * text and DETACH it so it stays posted after we leave foreground state, then stop the service. + * Leaving foreground state is safe on the [onStartCommand] paths too (never-foregrounded there, so + * `stopForeground` is a no-op), and [stopSelf] must run promptly because that start arrived via + * `startForegroundService` — otherwise the platform raises the "did not call startForeground in + * time" ANR. [stopSelf] then tears down [scope] in [onDestroy], closing the IDLE connections; mail + * arrives via the 15-minute periodic sync until push is started again (next app foreground / cap + * reset). */ // Permission is checked via hasNotificationPermission() below; lint can't trace the indirect guard. @SuppressLint("MissingPermission") - private fun fallBackToPeriodicSync() { - AppLog.i(TAG, "dataSync FGS runtime cap reached: pausing IMAP IDLE; mail arrives via 15-minute periodic sync") + private fun degradeToPeriodicSync() { // Already scheduled at every app start (UPDATE, so a no-op here) — re-asserted so the fallback // provably exists now that push is paused, mirroring the low-battery path in onPushModeChanged. syncScheduler.schedulePeriodicSync() @@ -187,6 +235,25 @@ class IdleService : Service() { stopSelf() } + /** Records the wall-independent time of the last dataSync cap event, arming [capWindowActive]. */ + private fun markCapReached() { + capReachedElapsedMs = SystemClock.elapsedRealtime() + } + + /** + * True while still within [CAP_WINDOW_MS] of the last cap event ([markCapReached]) — a burst of + * restarts in that window is certainly still capped, so [onStartCommand] skips the foreground start + * (and its now-guaranteed rejection) entirely. The window is anchored to the last real cap event + * and never refreshed by the skip itself, so it expires and lets a later restart re-probe; that + * probe is safe because a still-capped rejection is caught. Uses [SystemClock.elapsedRealtime] (not + * wall-clock) so it is immune to clock changes, and the companion field survives service + * re-creation within the process (which is where the restart storm happens). + */ + private fun capWindowActive(): Boolean { + val reachedAt = capReachedElapsedMs + return reachedAt != 0L && SystemClock.elapsedRealtime() - reachedAt < CAP_WINDOW_MS + } + private fun hasNotificationPermission(): Boolean = ContextCompat.checkSelfPermission(this, Manifest.permission.POST_NOTIFICATIONS) == PackageManager.PERMISSION_GRANTED @@ -246,5 +313,17 @@ class IdleService : Service() { // Re-establish IDLE on this cadence — under RFC 2177's 29-minute ceiling and short enough // to beat typical NAT/firewall idle-socket timeouts. const val IDLE_RENEWAL_MS = 9 * 60_000L + + // How long after a dataSync cap event onStartCommand skips the (still-illegal) foreground start + // outright (#354). The true rolling-24h budget reset is unknowable client-side, so this is a + // restart-storm damper, not a precise predictor: it matches the periodic-sync interval — the + // fallback already covering mail — so at most one foreground-start probe happens per cycle, and + // re-probing after it is safe because a still-capped rejection is caught, not fatal. + const val CAP_WINDOW_MS = 15 * 60_000L + + // elapsedRealtime() of the last dataSync cap event; 0 = none this process. Companion-scoped so + // it survives IdleService re-creation within the process, where the restart storm happens. + @Volatile + private var capReachedElapsedMs = 0L } } diff --git a/app/src/test/kotlin/org/libremail/push/IdleForegroundStarterTest.kt b/app/src/test/kotlin/org/libremail/push/IdleForegroundStarterTest.kt new file mode 100644 index 0000000..cda84ed --- /dev/null +++ b/app/src/test/kotlin/org/libremail/push/IdleForegroundStarterTest.kt @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.push + +import android.app.Service +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertSame +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * JVM coverage of [IdleForegroundStarter] — the dataSync foreground-start decision + * `IdleService.onStartCommand` delegates to (#354). Extracted out of the Android `Service` precisely + * so this branchy logic — skip while capped, catch the runtime-cap + * `ForegroundServiceStartNotAllowedException` (surfaced via its [IllegalStateException] supertype) and + * degrade, otherwise proceed — is unit-testable with no emulator and no Robolectric (this repo has + * neither). It reads only the `Service.START_NOT_STICKY` constant; no `Service` is instantiated. + */ +class IdleForegroundStarterTest { + + @Test + fun `a successful foreground start proceeds to onStarted and returns START_NOT_STICKY`() { + var started = false + var degradeCalled = false + + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = false, + enterForeground = { /* startForeground succeeds */ }, + onStarted = { started = true }, + onDegraded = { degradeCalled = true }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertTrue("onStarted must run after a successful foreground start", started) + assertFalse("degrade must not run when the start succeeds", degradeCalled) + } + + @Test + fun `a runtime-cap rejection is caught, routed to degrade with the cause, and never propagates`() { + // The exact platform rejection is a ForegroundServiceStartNotAllowedException (API 31+); here we + // throw its IllegalStateException supertype, which is what the seam catches (and what the stub + // android.jar lets us construct on the JVM). + val rejection = IllegalStateException("Time limit already exhausted for foreground service type dataSync") + var started = false + var degradedWith: Throwable? = null + + // No exception escapes — that is the whole point of the fix (was an uncaught crash → restart loop). + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = false, + enterForeground = { throw rejection }, + onStarted = { started = true }, + onDegraded = { cause -> degradedWith = cause }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertFalse("a rejected foreground start must not begin IDLE watching", started) + assertSame("the rejection cause must reach the degrade path", rejection, degradedWith) + } + + @Test + fun `an active cap window skips the foreground start entirely and degrades with no cause`() { + var enterForegroundAttempted = false + var started = false + var degradeCalled = false + var degradedWith: Throwable? = null + + val result = IdleForegroundStarter.startForegroundOrDegrade( + capActive = true, + enterForeground = { enterForegroundAttempted = true }, + onStarted = { started = true }, + onDegraded = { cause -> + degradeCalled = true + degradedWith = cause + }, + ) + + assertEquals(Service.START_NOT_STICKY, result) + assertFalse("must not attempt a dataSync FGS start while still inside the cap window", enterForegroundAttempted) + assertFalse(started) + assertTrue("must still degrade to periodic sync while capped", degradeCalled) + assertNull("the cap-window skip carries no throwable cause", degradedWith) + } + + @Test + fun `a non-IllegalStateException from the foreground start propagates unchanged`() { + // Only the runtime-cap/background ISE is a safe-degrade condition; anything else (e.g. a + // SecurityException) is a genuine bug we must not swallow. + val boom = SecurityException("not an FGS runtime-cap rejection") + var degradeCalled = false + + val thrown = runCatching { + IdleForegroundStarter.startForegroundOrDegrade( + capActive = false, + enterForeground = { throw boom }, + onStarted = {}, + onDegraded = { degradeCalled = true }, + ) + }.exceptionOrNull() + + assertSame("an unrelated failure must propagate, not degrade", boom, thrown) + assertFalse("degrade must not run for a non-ISE failure", degradeCalled) + } +} -- 2.47.3