Merge pull request #366 from JMR-dev/fix-354-idleservice-fgs

fix(push): stop IdleService dataSync FGS crash-loop on exhausted 24h cap (#354)

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<Context>()
+
+    @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)
+    }
+}

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
+    }
+}

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<String, Job>()
 
-    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
     }
 }

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)
+    }
+}