fix(#516): review round 3 - change runs stay off the status, unlisted copies don't retire the old layout, stable export order for the hash

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

@@ -32,6 +32,7 @@ import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs
 import de.jeanlucmakiola.calendula.domain.backup.BackupFilePlan
 import de.jeanlucmakiola.calendula.domain.backup.backupFingerprint
 import de.jeanlucmakiola.calendula.domain.backup.backupTargets
+import de.jeanlucmakiola.calendula.domain.backup.inBackupOrder
 import de.jeanlucmakiola.calendula.domain.backup.needsRefresh
 import de.jeanlucmakiola.calendula.domain.backup.withFingerprint
 import de.jeanlucmakiola.calendula.domain.backup.planBackupFiles
@@ -185,9 +186,9 @@ class BackupWorker(
                 prefs.autoBackupFolderUri.first() != null
             if (wanted) BackupScheduler.watchChanges(applicationContext, slot = 1 - slot)
         }
+        runCatching { watchNext() }.onFailure { Log.w(TAG, "Arming the next change run failed", it) }
         // Never fails or retries: a retry would only wait for another change.
         try {
-            watchNext()
             backUp(deps, prefs, changeRun = true)
         } catch (e: CancellationException) {
             throw e
@@ -201,8 +202,8 @@ class BackupWorker(
 
     /**
      * One run under the shared lock. A change run (#516) refreshes only the
-     * stable copies; it reports a failure, but a success stays off the status
-     * line, which dates the last dated backup.
+     * stable copies and stays off the status line and the failure count, which
+     * report on the scheduled backup; a broken folder fails that one too.
      */
     private suspend fun backUp(deps: Deps, prefs: SettingsPrefs, changeRun: Boolean): Result {
         val now = System.currentTimeMillis()
@@ -223,9 +224,8 @@ class BackupWorker(
                 RunOutcome.Failed
             }
         }
-        if (outcome == RunOutcome.NothingToDo) return Result.success()
+        if (changeRun || outcome == RunOutcome.NothingToDo) return Result.success()
         val success = outcome == RunOutcome.Written
-        if (changeRun && success) return Result.success()
         prefs.recordAutoBackupRun(success = success, atMillis = now)
         if (!success && prefs.autoBackupStatus.first().consecutiveFailures >= FAILURE_NOTIFY_THRESHOLD) {
             notifyFailure(applicationContext)
@@ -239,7 +239,8 @@ class BackupWorker(
     /**
      * Writes each series and prunes it only once its new file has landed. A
      * [latestOnly] run (#516) leaves the dated files and their retention to the
-     * schedule and rewrites only the stable copies whose calendars changed.
+     * schedule and rewrites only the stable copies whose calendars changed;
+     * its outcome isn't reported.
      */
     private suspend fun runBackup(deps: Deps, prefs: SettingsPrefs, folder: Uri, latestOnly: Boolean): RunOutcome {
         val repository = deps.repository()
@@ -307,9 +308,7 @@ class BackupWorker(
         } catch (e: Exception) {
             Log.w(TAG, "Stable copy of one series failed", e)
             // The copy may be half-written now: the next change run must not take it as current.
-            runCatching {
-                prefs.updateAutoBackupLedger(folder.toString(), fingerprints = { it - plan.target.key }) { it }
-            }
+            runCatching { prefs.dropAutoBackupFingerprint(folder.toString(), plan.target.key) }
             false
         }
 
@@ -320,7 +319,8 @@ class BackupWorker(
             val target = plan.target
             if (latestOnly && plan.latestName == null) continue
             val text = try {
-                IcsWriter().writeCalendar(repository.exportEvents(target.calendarIds), stamp, target.calendarName)
+                val events = repository.exportEvents(target.calendarIds).inBackupOrder()
+                IcsWriter().writeCalendar(events, stamp, target.calendarName)
             } catch (e: CancellationException) {
                 throw e
             } catch (e: Exception) {
@@ -330,18 +330,18 @@ class BackupWorker(
             }
             val fingerprint = backupFingerprint(text)
             if (latestOnly && !plan.needsRefresh(fingerprint, fingerprints, listing.files.keys, listing.complete)) {
-                landedLatest += target.key
+                if (plan.latestName in listing.files) landedLatest += target.key
                 continue
             }
             val bytes = text.toByteArray(Charsets.UTF_8)
             // Not cancellable once a file exists: it must reach the ledger, or
             // nothing would ever prune it.
             withContext(NonCancellable) {
-                val dated = latestOnly || writeDated(plan, bytes)
-                // A full run's stable copy is best-effort and never holds up its status.
-                val latest = plan.latestName != null && writeLatest(plan, bytes, fingerprint)
-                if (latest) landedLatest += target.key
-                if (if (latestOnly) latest else dated) written++ else failed++
+                if (!latestOnly) {
+                    if (writeDated(plan, bytes)) written++ else failed++
+                }
+                // Best-effort in a full run: it never holds up the run's status.
+                if (plan.latestName != null && writeLatest(plan, bytes, fingerprint)) landedLatest += target.key
             }
         }
         // A layout switch retires the other layout's copies once every new one is there.

app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefs.kt

@@ -399,6 +399,15 @@ class SettingsPrefs @Inject constructor(
         }
     }
 
+    /** Forget [key]'s hash (#516), so the next change run rewrites its stable copy. */
+    suspend fun dropAutoBackupFingerprint(folder: String, key: String) {
+        prefs.edit {
+            if (it[SettingsKeys.autoBackupFolderUri] == folder) {
+                it[SettingsKeys.autoBackupFingerprints] = it[SettingsKeys.autoBackupFingerprints] - key
+            }
+        }
+    }
+
     /** Record a pre-#411 install's old file as its combined series' stable copy (see [pinBackupLayout]). */
     suspend fun adoptLegacyBackupFile() {
         prefs.edit { if (it.autoBackupLegacyInstall) it.adoptLegacyBackupFile() }

app/src/main/java/de/jeanlucmakiola/calendula/domain/backup/BackupPlan.kt

@@ -1,6 +1,7 @@
 package de.jeanlucmakiola.calendula.domain.backup
 
 import de.jeanlucmakiola.calendula.domain.CalendarSource
+import de.jeanlucmakiola.calendula.domain.ics.IcsEvent
 import de.jeanlucmakiola.calendula.domain.isNotSynced
 import de.jeanlucmakiola.floret.android.safeFileName
 import java.security.MessageDigest
@@ -324,8 +325,8 @@ fun parseBackupFingerprints(stored: String?): BackupFingerprints =
 
 /**
  * Whether a run started by a change (#516) rewrites this series' stable copy:
- * only when its content moved on from what the copy holds, or the copy is
- * missing or mid-rename. Never when the name belongs to a file of the user's,
+ * only when its content moved on from what the copy holds (a rename changes
+ * the name written into it), or the copy is missing. Never when the name belongs to a file of the user's,
  * nor when a [complete]-less listing doesn't show the copy: writing then would
  * add a duplicate beside it, so that is left to the scheduled run.
  */
@@ -337,5 +338,13 @@ fun BackupFilePlan.needsRefresh(
 ): Boolean {
     val latest = latestName ?: return false
     if (latest !in listed) return complete
-    return stored[target.key] != fingerprint || staleLatest.isNotEmpty() || handOff.isNotEmpty()
+    return stored[target.key] != fingerprint
 }
+
+/**
+ * The events in a fixed order, reminders sorted, so the same content always
+ * writes the same file and hashes the same, however the provider returned it.
+ */
+fun List<IcsEvent>.inBackupOrder(): List<IcsEvent> =
+    sortedWith(compareBy<IcsEvent>({ it.start }, { it.uid }, { it.overrideAt }))
+        .map { it.copy(reminderMinutes = it.reminderMinutes.sorted()) }

app/src/test/java/de/jeanlucmakiola/calendula/domain/backup/BackupPlanTest.kt

@@ -330,7 +330,6 @@ class BackupPlanTest {
         assertThat(plan.needsRefresh("h2", stored, listed, complete = true)).isTrue()
         assertThat(plan.needsRefresh("h1", emptyMap(), listed, complete = true)).isTrue()
         assertThat(plan.needsRefresh("h1", stored, emptySet(), complete = true)).isTrue()
-        assertThat(plan.copy(staleLatest = listOf("Job.ics")).needsRefresh("h1", stored, listed, complete = true)).isTrue()
     }
 
     @Test
@@ -349,4 +348,16 @@ class BackupPlanTest {
         assertThat(plan.latestName).isNull()
         assertThat(plan.needsRefresh("h1", emptyMap(), setOf("Work.ics"), complete = true)).isFalse()
     }
+
+    @Test
+    fun `backup order doesn't depend on the provider's`() {
+        fun event(uid: String, reminders: List<Int>) = IcsEvent(
+            uid = uid, summary = uid, start = Instant.fromEpochSeconds(0), end = Instant.fromEpochSeconds(3600),
+            isAllDay = false, zoneId = "UTC", reminderMinutes = reminders,
+        )
+        val one = listOf(event("b", listOf(15, 10)), event("a", listOf(5)))
+        val other = listOf(event("a", listOf(5)), event("b", listOf(10, 15)))
+        assertThat(one.inBackupOrder()).isEqualTo(other.inBackupOrder())
+        assertThat(one.inBackupOrder().map { it.uid }).containsExactly("a", "b").inOrder()
+    }
 }

docs/ARCHITECTURE.md

@@ -492,8 +492,8 @@ 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 again as it ends. Runs share the
-scheduled backup's lock. A change run only rewrites the stable copies and shows
-on the status line only when it fails: the dated files and their retention stay on the interval,
+scheduled backup's lock. A change run only rewrites the stable copies and stays
+off the status line: the dated files and their retention stay on the interval,
 so a day of edits can't rotate the history out. Since the trigger fires for any
 provider change (reminders, other calendars, sync bookkeeping), each series'
 file is hashed without its `DTSTAMP`s (`auto_backup_fingerprints`, reset with