fix(#516): review round 10 - the change watch is one JobScheduler job that re-arms itself, startup reconcile can't crash the app

app/src/main/AndroidManifest.xml

@@ -304,6 +304,11 @@
              wipe pending alarms, and a clock or timezone change moves every
              reminder relative to the one that is armed.
              Exported: the system broadcasts arrive from outside the app. -->
+        <service
+            android:name=".data.backup.BackupChangeJob"
+            android:exported="false"
+            android:permission="android.permission.BIND_JOB_SERVICE" />
+
         <receiver
             android:name=".data.reminders.ReminderScheduleReceiver"
             android:exported="true">

app/src/main/java/de/jeanlucmakiola/calendula/data/backup/AutoBackup.kt

@@ -7,7 +7,6 @@ import android.app.PendingIntent
 import android.content.Context
 import android.net.Uri
 import android.os.Build
-import android.provider.CalendarContract
 import android.util.Log
 import androidx.core.app.NotificationCompat
 import androidx.core.app.NotificationManagerCompat
@@ -66,14 +65,8 @@ object BackupScheduler {
 
     private const val WORK_NAME = "auto-backup"
     private const val WORK_NAME_NOW = "auto-backup-now"
-    private val CHANGE_SLOTS = listOf("auto-backup-changes-a", "auto-backup-changes-b")
-    internal const val KEY_CHANGE_SLOT = "change_slot"
-
-    /** Quiet time after the last provider change before a change run starts, so a burst makes one run. */
-    private const val CHANGE_SETTLE_MINUTES = 2L
-
-    /** Longest a change run waits on a provider that never goes quiet (a long sync). */
-    private const val CHANGE_MAX_WAIT_MINUTES = 15L
+    private const val WORK_NAME_CHANGE = "auto-backup-change"
+    internal const val KEY_CHANGE_RUN = "change_run"
 
     private val reconcileLock = Mutex()
 
@@ -84,67 +77,70 @@ object BackupScheduler {
      *
      * The first run is delayed by one interval so it doesn't follow an immediate
      * [runNow] with a near-identical copy. With backing up after changes on
-     * (#516), the change trigger is armed unless a slot is already waiting.
+     * (#516), the change watch ([BackupChangeJob]) is armed unless it already
+     * waits; it doesn't survive a reboot, and this runs on every process start.
      * Serialised and not cancellable, so quick toggles settle on the last one.
      */
     suspend fun reconcile(context: Context, prefs: SettingsPrefs) = withContext(NonCancellable) {
         reconcileLock.withLock {
-            val workManager = WorkManager.getInstance(context)
-            suspend fun cancelWatch() = CHANGE_SLOTS.indices
-                .filter { slot -> slotStates(workManager, slot).any { !it.isFinished } }
-                .forEach { workManager.cancelUniqueWork(CHANGE_SLOTS[it]).await() }
-            if (!prefs.autoBackupEnabled.first() || prefs.autoBackupFolderUri.first() == null) {
-                workManager.cancelUniqueWork(WORK_NAME)
-                // Also cancel any pending immediate run — otherwise a failing run-now
-                // keeps retrying after the user has turned backup off.
-                workManager.cancelUniqueWork(WORK_NAME_NOW)
-                cancelWatch()
-                return@withLock
+            try {
+                apply(context, prefs)
+            } catch (e: Exception) {
+                Log.w(TAG, "Scheduling the automatic backup failed", e)
             }
-            // WorkManager's own floor is 15 min; the UI floors the user choice at 30.
-            val interval = prefs.autoBackupIntervalMinutes.first().coerceAtLeast(15L)
-            val request = PeriodicWorkRequestBuilder<BackupWorker>(interval, TimeUnit.MINUTES)
-                .setInitialDelay(interval, TimeUnit.MINUTES)
-                .build()
-            workManager.enqueueUniquePeriodicWork(
-                WORK_NAME,
-                ExistingPeriodicWorkPolicy.UPDATE,
-                request,
-            )
-            if (!prefs.autoBackupOnChange.first()) {
-                cancelWatch()
-                return@withLock
-            }
-            takeWatchStep(context, workManager, self = null)
         }
     }
 
+    private suspend fun apply(context: Context, prefs: SettingsPrefs) {
+        val workManager = WorkManager.getInstance(context)
+        fun stopWatching() {
+            BackupChangeJob.cancel(context)
+            workManager.cancelUniqueWork(WORK_NAME_CHANGE)
+        }
+        if (!prefs.autoBackupEnabled.first() || prefs.autoBackupFolderUri.first() == null) {
+            workManager.cancelUniqueWork(WORK_NAME)
+            // Also cancel any pending immediate run — otherwise a failing run-now
+            // keeps retrying after the user has turned backup off.
+            workManager.cancelUniqueWork(WORK_NAME_NOW)
+            stopWatching()
+            return
+        }
+        // WorkManager's own floor is 15 min; the UI floors the user choice at 30.
+        val interval = prefs.autoBackupIntervalMinutes.first().coerceAtLeast(15L)
+        val request = PeriodicWorkRequestBuilder<BackupWorker>(interval, TimeUnit.MINUTES)
+            .setInitialDelay(interval, TimeUnit.MINUTES)
+            .build()
+        workManager.enqueueUniquePeriodicWork(
+            WORK_NAME,
+            ExistingPeriodicWorkPolicy.UPDATE,
+            request,
+        )
+        if (prefs.autoBackupOnChange.first()) BackupChangeJob.scheduleIfIdle(context) else stopWatching()
+    }
+
     /**
-     * Keep one change watch armed around the change run in [self] (see
-     * [watchStep]), under the same lock as [reconcile] so it can't outlast a
-     * switch-off.
+     * The watch fired: queue a change run and arm the next watch, unless the
+     * option went off meanwhile. Under [reconcileLock], so it can't re-arm
+     * after a switch-off.
      */
-    internal suspend fun watchNext(context: Context, prefs: SettingsPrefs, self: Int) =
-        withContext(NonCancellable) {
-            reconcileLock.withLock {
-                if (watchWanted(prefs)) takeWatchStep(context, WorkManager.getInstance(context), self)
+    internal suspend fun changed(context: Context, prefs: SettingsPrefs) = withContext(NonCancellable) {
+        reconcileLock.withLock {
+            val wanted = prefs.autoBackupEnabled.first() && prefs.autoBackupFolderUri.first() != null &&
+                prefs.autoBackupOnChange.first()
+            if (!wanted) return@withLock
+            val workManager = WorkManager.getInstance(context)
+            val states = workManager.getWorkInfosForUniqueWorkFlow(WORK_NAME_CHANGE).first().map { it.state }
+            changeRunPolicy(states)?.let { policy ->
+                val run = OneTimeWorkRequestBuilder<BackupWorker>()
+                    .setConstraints(Constraints.Builder().setRequiresBatteryNotLow(true).build())
+                    .setInputData(workDataOf(KEY_CHANGE_RUN to true))
+                    .build()
+                workManager.enqueueUniqueWork(WORK_NAME_CHANGE, policy, run).await()
             }
-        }
-
-    private suspend fun takeWatchStep(context: Context, workManager: WorkManager, self: Int?) {
-        when (val step = watchStep(self, CHANGE_SLOTS.indices.map { slotStates(workManager, it) })) {
-            WatchStep.None -> Unit
-            is WatchStep.Cancel -> workManager.cancelUniqueWork(CHANGE_SLOTS[step.slot]).await()
-            is WatchStep.Arm -> watchChanges(context, step.slot, step.append)
+            BackupChangeJob.schedule(context)
         }
     }
 
-    private suspend fun slotStates(workManager: WorkManager, slot: Int): List<WorkInfo.State> =
-        workManager.getWorkInfosForUniqueWorkFlow(CHANGE_SLOTS[slot]).first().map { it.state }
-
-    private suspend fun watchWanted(prefs: SettingsPrefs): Boolean =
-        prefs.autoBackupEnabled.first() && prefs.autoBackupFolderUri.first() != null && prefs.autoBackupOnChange.first()
-
     /**
      * Run one export immediately (e.g. right after enabling or changing the
      * folder) for instant feedback. Unique + REPLACE so rapid taps coalesce into
@@ -158,58 +154,18 @@ object BackupScheduler {
         )
     }
 
-    /**
-     * Arm a one-off run on the next change in the calendar provider (#516) in
-     * one of two [slot]s. The provider reports every change on its root URI,
-     * whoever made it (this app, a sync adapter or another app), so that is the
-     * URI watched. A content trigger fires once and observes only while its
-     * job waits, so each change run arms the other slot ([watchNext]); [append]
-     * queues the request behind that slot's running run.
-     */
-    private fun watchChanges(context: Context, slot: Int, append: Boolean) {
-        val constraints = Constraints.Builder()
-            .addContentUriTrigger(CalendarContract.CONTENT_URI, true)
-            .setTriggerContentUpdateDelay(CHANGE_SETTLE_MINUTES, TimeUnit.MINUTES)
-            .setTriggerContentMaxDelay(CHANGE_MAX_WAIT_MINUTES, TimeUnit.MINUTES)
-            .setRequiresBatteryNotLow(true)
-            .build()
-        val request = OneTimeWorkRequestBuilder<BackupWorker>()
-            .setConstraints(constraints)
-            .setInputData(workDataOf(KEY_CHANGE_SLOT to slot))
-            .build()
-        val policy = if (append) ExistingWorkPolicy.APPEND_OR_REPLACE else ExistingWorkPolicy.KEEP
-        WorkManager.getInstance(context).enqueueUniqueWork(CHANGE_SLOTS[slot], policy, request)
-    }
-}
-
-/** What keeps exactly one change watch armed (#516): nothing, a new watch in [slot], or cancelling the extra one there. */
-internal sealed interface WatchStep {
-    data object None : WatchStep
-
-    /** [append]: queued behind the run still going in [slot], so it starts watching once that ends. */
-    data class Arm(val slot: Int, val append: Boolean) : WatchStep
-
-    data class Cancel(val slot: Int) : WatchStep
+    private const val TAG = "BackupScheduler"
 }
 
 /**
- * The step that leaves exactly one watch waiting, from both slots' work
- * [states], taken by the change run in [self] as it starts and ends, or by
- * [BackupScheduler.reconcile] (null). A waiting watch is enough; two are one
- * too many. With none, the other slot is armed, queued behind its run if one
- * still shows as running, so a run whose end isn't recorded yet can't leave
- * the provider unwatched.
+ * How a fired change watch queues its run (#516), from the change runs' work
+ * [states]: a run that hasn't started yet will see the change anyway (null);
+ * one already going may have read too early, so the new one queues behind it.
  */
-internal fun watchStep(self: Int?, states: List<List<WorkInfo.State>>): WatchStep {
-    fun waiting(slot: Int) = states[slot].any { it == WorkInfo.State.ENQUEUED || it == WorkInfo.State.BLOCKED }
-    val keep = self ?: 0
-    val other = 1 - keep
-    val target = if (self == null) keep else other
-    return when {
-        waiting(keep) && waiting(other) -> WatchStep.Cancel(other)
-        waiting(keep) || waiting(other) -> WatchStep.None
-        else -> WatchStep.Arm(target, append = WorkInfo.State.RUNNING in states[target])
-    }
+internal fun changeRunPolicy(states: List<WorkInfo.State>): ExistingWorkPolicy? = when {
+    states.any { it == WorkInfo.State.ENQUEUED || it == WorkInfo.State.BLOCKED } -> null
+    WorkInfo.State.RUNNING in states -> ExistingWorkPolicy.APPEND_OR_REPLACE
+    else -> ExistingWorkPolicy.REPLACE
 }
 
 /**
@@ -235,24 +191,10 @@ class BackupWorker(
 
     override suspend fun doWork(): Result {
         val deps = EntryPointAccessors.fromApplication(applicationContext, Deps::class.java)
-        val prefs = deps.settingsPrefs()
-        val slot = inputData.getInt(BackupScheduler.KEY_CHANGE_SLOT, -1)
-        if (slot < 0) return backUp(deps, prefs, changeRun = false)
-        suspend fun watchNext() = runCatching {
-            BackupScheduler.watchNext(applicationContext, prefs, slot)
-        }.onFailure { Log.w(TAG, "Arming the next change run failed", it) }
-        watchNext()
-        // Never fails or retries: a retry would only wait for another change.
-        try {
-            backUp(deps, prefs, changeRun = true)
-        } catch (e: CancellationException) {
-            throw e
-        } catch (e: Exception) {
-            Log.w(TAG, "Backup after a change failed", e)
-        } finally {
-            watchNext()
-        }
-        return Result.success()
+        val changeRun = inputData.getBoolean(BackupScheduler.KEY_CHANGE_RUN, false)
+        val result = backUp(deps, deps.settingsPrefs(), changeRun)
+        // A failed change run would fail the one queued behind it too.
+        return if (changeRun) Result.success() else result
     }
 
     /**

app/src/main/java/de/jeanlucmakiola/calendula/data/backup/BackupChangeJob.kt

@@ -0,0 +1,81 @@
+package de.jeanlucmakiola.calendula.data.backup
+
+import android.app.job.JobInfo
+import android.app.job.JobParameters
+import android.app.job.JobScheduler
+import android.app.job.JobService
+import android.content.ComponentName
+import android.content.Context
+import android.provider.CalendarContract
+import android.util.Log
+import dagger.hilt.android.EntryPointAccessors
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.SupervisorJob
+import kotlinx.coroutines.cancel
+import kotlinx.coroutines.launch
+import java.util.concurrent.TimeUnit
+
+/**
+ * The watch behind "Also back up after changes" (#516): a JobScheduler job
+ * with a content trigger on the provider's root URI, where it reports every
+ * change, whoever made it (this app, a sync adapter or another app). It waits
+ * for [SETTLE_MINUTES] of quiet, at most [MAX_WAIT_MINUTES], so a burst makes
+ * one run. A trigger fires once; each firing queues a change run and schedules
+ * the next watch under the same id before finishing, which hands over the
+ * changes seen meanwhile, so exactly one watch exists and none are lost.
+ */
+class BackupChangeJob : JobService() {
+
+    private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default)
+
+    override fun onStartJob(params: JobParameters): Boolean {
+        scope.launch {
+            try {
+                val deps = EntryPointAccessors.fromApplication(applicationContext, BackupWorker.Deps::class.java)
+                BackupScheduler.changed(applicationContext, deps.settingsPrefs())
+            } catch (e: Exception) {
+                Log.w(TAG, "Queueing a backup after a change failed", e)
+            } finally {
+                jobFinished(params, false)
+            }
+        }
+        return true
+    }
+
+    override fun onStopJob(params: JobParameters): Boolean = false
+
+    override fun onDestroy() {
+        scope.cancel()
+        super.onDestroy()
+    }
+
+    companion object {
+        /** Clear of WorkManager's ids, which count up from 0. */
+        private const val JOB_ID = 0x0516_0001
+        private const val SETTLE_MINUTES = 2L
+        private const val MAX_WAIT_MINUTES = 15L
+        private const val TAG = "BackupChangeJob"
+
+        fun schedule(context: Context) {
+            val job = JobInfo.Builder(JOB_ID, ComponentName(context, BackupChangeJob::class.java))
+                .addTriggerContentUri(
+                    JobInfo.TriggerContentUri(
+                        CalendarContract.CONTENT_URI,
+                        JobInfo.TriggerContentUri.FLAG_NOTIFY_FOR_DESCENDANTS,
+                    ),
+                )
+                .setTriggerContentUpdateDelay(TimeUnit.MINUTES.toMillis(SETTLE_MINUTES))
+                .setTriggerContentMaxDelay(TimeUnit.MINUTES.toMillis(MAX_WAIT_MINUTES))
+                .build()
+            context.getSystemService(JobScheduler::class.java).schedule(job)
+        }
+
+        /** Arm the watch unless it already waits; replacing it would drop the changes it has seen. */
+        fun scheduleIfIdle(context: Context) {
+            if (context.getSystemService(JobScheduler::class.java).getPendingJob(JOB_ID) == null) schedule(context)
+        }
+
+        fun cancel(context: Context) = context.getSystemService(JobScheduler::class.java).cancel(JOB_ID)
+    }
+}

app/src/test/java/de/jeanlucmakiola/calendula/data/backup/ChangeRunPolicyTest.kt

@@ -0,0 +1,30 @@
+package de.jeanlucmakiola.calendula.data.backup
+
+import androidx.work.ExistingWorkPolicy
+import androidx.work.WorkInfo.State.BLOCKED
+import androidx.work.WorkInfo.State.CANCELLED
+import androidx.work.WorkInfo.State.ENQUEUED
+import androidx.work.WorkInfo.State.RUNNING
+import androidx.work.WorkInfo.State.SUCCEEDED
+import com.google.common.truth.Truth.assertThat
+import org.junit.jupiter.api.Test
+
+class ChangeRunPolicyTest {
+
+    @Test
+    fun `a run that hasn't started yet covers the change`() {
+        assertThat(changeRunPolicy(listOf(ENQUEUED))).isNull()
+        assertThat(changeRunPolicy(listOf(RUNNING, BLOCKED))).isNull()
+    }
+
+    @Test
+    fun `a run already going gets the next one queued behind it`() {
+        assertThat(changeRunPolicy(listOf(RUNNING))).isEqualTo(ExistingWorkPolicy.APPEND_OR_REPLACE)
+    }
+
+    @Test
+    fun `with nothing pending a fresh run starts`() {
+        assertThat(changeRunPolicy(emptyList())).isEqualTo(ExistingWorkPolicy.REPLACE)
+        assertThat(changeRunPolicy(listOf(SUCCEEDED, CANCELLED))).isEqualTo(ExistingWorkPolicy.REPLACE)
+    }
+}

app/src/test/java/de/jeanlucmakiola/calendula/data/backup/WatchStepTest.kt

@@ -1,41 +0,0 @@
-package de.jeanlucmakiola.calendula.data.backup
-
-import androidx.work.WorkInfo.State.BLOCKED
-import androidx.work.WorkInfo.State.ENQUEUED
-import androidx.work.WorkInfo.State.RUNNING
-import androidx.work.WorkInfo.State.SUCCEEDED
-import com.google.common.truth.Truth.assertThat
-import org.junit.jupiter.api.Test
-
-class WatchStepTest {
-
-    @Test
-    fun `a lone run arms the idle slot and leaves a waiting one`() {
-        assertThat(watchStep(0, listOf(listOf(RUNNING), listOf(SUCCEEDED)))).isEqualTo(WatchStep.Arm(1, append = false))
-        assertThat(watchStep(0, listOf(listOf(RUNNING), listOf(ENQUEUED)))).isEqualTo(WatchStep.None)
-    }
-
-    @Test
-    fun `a run that still shows as running gets the watch queued behind it`() {
-        assertThat(watchStep(1, listOf(listOf(RUNNING), listOf(RUNNING)))).isEqualTo(WatchStep.Arm(0, append = true))
-    }
-
-    @Test
-    fun `runs that fired together end with one watch`() {
-        val afterFirst = listOf(listOf(RUNNING), listOf(RUNNING, BLOCKED))
-        assertThat(watchStep(1, afterFirst)).isEqualTo(WatchStep.None)
-        assertThat(watchStep(0, afterFirst)).isEqualTo(WatchStep.None)
-    }
-
-    @Test
-    fun `a second waiting watch is cancelled`() {
-        assertThat(watchStep(0, listOf(listOf(ENQUEUED), listOf(ENQUEUED)))).isEqualTo(WatchStep.Cancel(1))
-        assertThat(watchStep(null, listOf(listOf(ENQUEUED), listOf(ENQUEUED)))).isEqualTo(WatchStep.Cancel(1))
-    }
-
-    @Test
-    fun `reconcile arms slot 0 only when nothing waits`() {
-        assertThat(watchStep(null, listOf(emptyList(), emptyList()))).isEqualTo(WatchStep.Arm(0, append = false))
-        assertThat(watchStep(null, listOf(emptyList(), listOf(ENQUEUED)))).isEqualTo(WatchStep.None)
-    }
-}

docs/ARCHITECTURE.md

@@ -485,29 +485,29 @@ of the backup keeps its last one. The pre-#411 fixed `calendula-backup.ics` is
 the combined series' stable copy for the install that wrote it, and an
 install that already had a folder stays on the combined layout.
 
-**Backing up after a change (#516)** is opt-in. `BackupScheduler.watchChanges`
-arms a one-off `BackupWorker` with a WorkManager content-URI trigger on the
-provider's root URI (the provider notifies every change there, whoever made it:
-this app, a sync adapter or another app), a 2-minute settle delay capped at 15
-minutes, and battery-not-low. A trigger fires once and observes only while its
-job waits, so there are two slots: a change run arms the other one as it starts
-(catching changes made during the run) and checks again as it ends;
-`watchStep` keeps exactly one watch waiting, queueing a new one behind a run
-that still shows as running and cancelling a second one. Runs share the
-scheduled backup's lock. A change run only rewrites the stable copies and never
-retries: the dated files and their retention stay on the interval, so a day of
-edits can't rotate the history out. Every run that writes something is recorded
-on the status line; the failure notification comes once per failure streak, as
-it reaches the threshold, and the next success of any run clears it. Since the
-trigger fires for any provider change (reminders, other calendars, sync
-bookkeeping), each series' file (events in a fixed order) is hashed without its
-`DTSTAMP`s (`auto_backup_fingerprints`, reset with the folder and kept only
-while the option is on) and a change run
-skips the series whose hash (bound to the copy's name) and stable copy are
-unchanged, without even listing the folder when no hash moved; the hash is
-dropped before a copy is rewritten. A copy that went missing is restored by the
-next run that lists the folder; one a still-loading listing doesn't show, or
-one edited in place outside the app, is left to the next scheduled run.
+**Backing up after a change (#516)** is opt-in. `BackupChangeJob` is a
+JobScheduler job with a content trigger on the provider's root URI (the provider
+notifies every change there, whoever made it: this app, a sync adapter or
+another app), a 2-minute settle delay capped at 15 minutes. A trigger fires
+once, so each firing queues a change run (a unique `BackupWorker`, battery not
+low, queued behind one already running) and schedules the next watch under the
+same id before it finishes, which carries over the changes seen meanwhile. Such
+a job can't be persisted; `BackupScheduler.reconcile` re-arms it on every
+process start (the boot receivers start one), without replacing a waiting watch.
+Runs share the scheduled backup's lock. A change run only rewrites the stable
+copies and never retries: the dated files and their retention stay on the
+interval, so a day of edits can't rotate the history out. Every run that writes
+something is recorded on the status line; the failure notification comes once
+per failure streak, as it reaches the threshold, and the next success of any run
+clears it. Since the trigger fires for any provider change (reminders, other
+calendars, sync bookkeeping), each series' file (events in a fixed order) is
+hashed without its `DTSTAMP`s (`auto_backup_fingerprints`, reset with the folder
+and kept only while the option is on) and a change run skips the series whose
+hash (bound to the copy's name) and stable copy are unchanged, without even
+listing the folder when no hash moved; the hash is dropped before a copy is
+rewritten. A copy that went missing is restored by the next run that lists the
+folder; one a still-loading listing doesn't show, or one edited in place outside
+the app, is left to the next scheduled run.
 
 **A backup carries the series' deleted and moved occurrences.** `EXDATE` lives on
 the master row and is part of `EventExportProjection`; `IcsWriter` marks an