fix(#147, #411): review round 6 - ledger reset per folder, atomic ledger updates, stable copies replaced only once the new one landed

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

@@ -159,8 +159,9 @@ class BackupWorker(
         val selection = prefs.autoBackupSelection.first()
         val targets = backupTargets(all.filter(selection::selects), combined = prefs.autoBackupCombined.first())
         if (targets.isEmpty()) return RunOutcome.NothingSelected
+        prefs.adoptLegacyBackupFile()
         val listing = exporter.listFolder(folder)
-        var ledger = prefs.autoBackupLedger.first()
+        val ledger = prefs.autoBackupLedger.first()
         // A listing still loading can't tell a gone file from a late one.
         val existing = if (listing.complete) listing.files.keys else listing.files.keys + ledger.values.flatten()
         val plans = planBackupFiles(
@@ -169,15 +170,13 @@ class BackupWorker(
             existing = existing,
             at = LocalDateTime.now(),
             keep = prefs.autoBackupKeep.first(),
-            legacyOwned = prefs.autoBackupLegacyInstall.first(),
         )
         val stamp = Clock.System.now()
         val liveKeys = all.map { it.id.toString() }.toSet()
-        suspend fun record(key: String, files: List<String>) {
-            ledger = withLedgerEntry(ledger, key, files, liveKeys)
-            prefs.setAutoBackupLedger(ledger)
-        }
+        suspend fun record(key: String, files: List<String>) =
+            prefs.updateAutoBackupLedger { withLedgerEntry(it, key, files, liveKeys) }
         var written = 0
+        val landedLatest = mutableSetOf<String>()
         for (plan in plans) {
             val target = plan.target
             val bytes = try {
@@ -202,28 +201,30 @@ class BackupWorker(
                 } catch (e: Exception) {
                     Log.w(TAG, "Backup of one series failed", e)
                 }
-                // The stable copy is best-effort: it never holds up the dated
-                // backup, its pruning or the run's status.
+                // Best-effort: the stable copy never holds up the dated backup,
+                // its pruning or the run's status. An old copy goes only once the
+                // new one has landed.
                 try {
+                    val latest = plan.latestName ?: return@withContext
                     val latestKey = latestLedgerKey(target.key)
-                    val latest = plan.latestName?.let { intended ->
-                        val landed = exporter.writeToFolder(folder, listing, intended, bytes)
-                        // A provider that renamed it leaves a copy to clean up next run.
-                        listOf(landed, intended).distinct()
-                    }.orEmpty()
-                    val owned = ledger[latestKey].orEmpty().filter { it !in plan.staleLatest }
-                    record(latestKey, plan.staleLatest + (latest.ifEmpty { owned }))
+                    // A provider that renamed it leaves a copy to clean up next run.
+                    val landed = exporter.writeToFolder(folder, listing, latest, bytes)
+                    record(latestKey, plan.staleLatest + landed)
+                    if (landed == latest) landedLatest += target.key
                     val staleLeft = exporter.deleteFromFolder(listing, plan.staleLatest)
-                    record(latestKey, plan.staleLatest.filter { it in staleLeft } + latest.ifEmpty { owned })
+                    record(latestKey, plan.staleLatest.filter { it in staleLeft } + landed)
                 } catch (e: Exception) {
                     Log.w(TAG, "Stable copy of one series failed", e)
                 }
             }
         }
-        replacedLatestCopies(targets, ledger, existing).forEach { (key, names) ->
-            withContext(NonCancellable) {
-                val left = exporter.deleteFromFolder(listing, names)
-                record(latestLedgerKey(key), names.filter { it in left })
+        // A layout switch retires the other layout's copies once every new one is there.
+        if (landedLatest.size == plans.size) {
+            replacedLatestCopies(targets, prefs.autoBackupLedger.first(), existing).forEach { (key, names) ->
+                withContext(NonCancellable) {
+                    val left = exporter.deleteFromFolder(listing, names)
+                    record(latestLedgerKey(key), names.filter { it in left })
+                }
             }
         }
         return when (written) {

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

@@ -354,6 +354,9 @@ class SettingsPrefs @Inject constructor(
     suspend fun setAutoBackupFolderUri(uri: String?) {
         prefs.edit {
             it.pinBackupLayout()
+            // The ledger says which files in the old folder are ours; in a new
+            // one a file of the same name may be the user's.
+            if (it[SettingsKeys.autoBackupFolderUri] != uri) it[SettingsKeys.autoBackupLedger] = emptyMap()
             it[SettingsKeys.autoBackupFolderUri] = uri
         }
     }
@@ -367,7 +370,15 @@ class SettingsPrefs @Inject constructor(
 
     suspend fun setAutoBackupKeep(keep: Int) = prefs.set(SettingsKeys.autoBackupKeep, keep)
 
-    suspend fun setAutoBackupLedger(ledger: BackupLedger) = prefs.set(SettingsKeys.autoBackupLedger, ledger)
+    /** Atomic, so a run's ledger writes never undo one made meanwhile (pinning the layout). */
+    suspend fun updateAutoBackupLedger(transform: (BackupLedger) -> BackupLedger) {
+        prefs.update(SettingsKeys.autoBackupLedger, transform)
+    }
+
+    /** 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() }
+    }
 
     /** Switch [calendar] in or out of the automatic backup (#147). */
     suspend fun setAutoBackupCalendar(calendar: CalendarSource, included: Boolean) {
@@ -839,12 +850,14 @@ val Preferences.autoBackupCombined: Boolean
 private fun MutablePreferences.pinBackupLayout() {
     if (this[SettingsKeys.autoBackupCombinedStored] != null) return
     val legacy = autoBackupLegacyInstall
+    if (legacy) adoptLegacyBackupFile()
     this[SettingsKeys.autoBackupCombinedStored] = legacy
-    if (legacy) {
-        val key = latestLedgerKey(COMBINED_SERIES)
-        val ledger = this[SettingsKeys.autoBackupLedger]
-        this[SettingsKeys.autoBackupLedger] = ledger + (key to (ledger[key].orEmpty() + LEGACY_BACKUP_FILE).distinct())
-    }
+}
+
+private fun MutablePreferences.adoptLegacyBackupFile() {
+    val key = latestLedgerKey(COMBINED_SERIES)
+    val ledger = this[SettingsKeys.autoBackupLedger]
+    this[SettingsKeys.autoBackupLedger] = ledger + (key to (ledger[key].orEmpty() + LEGACY_BACKUP_FILE).distinct())
 }
 
 /** An install that set up automatic backup before #411 and hasn't been pinned since. */

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

@@ -158,9 +158,7 @@ fun backupFileName(baseName: String, at: LocalDateTime): String =
  * files no longer in the folder ([existing]) are forgotten. Each target also
  * gets its stable copy, which retention never touches. A stable name is only
  * written when the file is absent or this series owns it — or a series of this
- * run that is moving off it (a rename hand-off). [legacyOwned]: this install
- * wrote the pre-#411 [LEGACY_BACKUP_FILE], which is the combined series' stable
- * copy now.
+ * run that is moving off it (a rename hand-off).
  */
 fun planBackupFiles(
     targets: List<BackupTarget>,
@@ -168,7 +166,6 @@ fun planBackupFiles(
     existing: Set<String>,
     at: LocalDateTime,
     keep: Int,
-    legacyOwned: Boolean = false,
 ): List<BackupFilePlan> {
     val retain = keep.coerceAtLeast(1)
     val runKeys = targets.map { it.key }.toSet()
@@ -180,13 +177,11 @@ fun planBackupFiles(
         val fileName = backupFileName(target.baseName, at)
         val latest = latestFileName(target.baseName)
         val earlier = ledger[target.key].orEmpty()
-            .filter { it in existing && it != fileName && it != latest && it != LEGACY_BACKUP_FILE }
+            .filter { it in existing && it != fileName && it != latest }
             .distinct()
         val kept = earlier.takeLast(retain - 1)
         val owner = owners[latest]
-        val writable = latest !in existing ||
-            (owner != null && owner in runKeys) ||
-            (latest == LEGACY_BACKUP_FILE && target.key == COMBINED_SERIES && legacyOwned)
+        val writable = latest !in existing || (owner != null && owner in runKeys)
         BackupFilePlan(
             target = target,
             fileName = fileName,
@@ -240,6 +235,12 @@ fun withLedgerEntry(
         series in liveKeys || series == COMBINED_SERIES
     }.toMutableMap()
     next[key] = files.distinct()
+    // A stable copy has one owner: claiming it takes it off any other series.
+    if (key.startsWith(LATEST_PREFIX)) {
+        next.keys.filter { it != key && it.startsWith(LATEST_PREFIX) }.forEach { other ->
+            next[other] = next.getValue(other) - files.toSet()
+        }
+    }
     return next.filterValues { it.isNotEmpty() }
 }
 

app/src/main/res/values/strings.xml

@@ -912,7 +912,7 @@
         <item quantity="one">Only the latest</item>
         <item quantity="other">The latest %d</item>
     </plurals>
-    <string name="calendars_auto_backup_keep_hint">Counted per file: with one file per calendar, each calendar keeps this many dated copies, plus one undated copy that always holds the latest backup. Older backups Calendula wrote are deleted after each run; a calendar you stop backing up keeps its last files, and nothing else in the folder is touched.</string>
+    <string name="calendars_auto_backup_keep_hint">Counted per file: with one file per calendar, each calendar keeps this many dated copies, plus one undated copy of the latest backup unless a file of yours already has that name. Older backups Calendula wrote are deleted after each run; a calendar you stop backing up keeps its last files, and nothing else in the folder is touched.</string>
     <string name="calendars_auto_backup_status_never">No automatic backup yet</string>
     <string name="calendars_auto_backup_status_ok">Last backup: %1$s</string>
     <string name="calendars_auto_backup_status_failed">Last backup failed: %1$s</string>

app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefsTest.kt

@@ -733,8 +733,8 @@ class SettingsPrefsTest {
         prefs.setAutoBackupFolderUri("content://tree/new")
         assertThat(prefs.autoBackupCombined.first()).isTrue()
         assertThat(prefs.autoBackupLegacyInstall.first()).isFalse()
-        assertThat(prefs.autoBackupLedger.first())
-            .containsExactly("latest-combined", listOf("calendula-backup.ics"))
+        // A new folder starts a new ledger: the old file there isn't known to be ours.
+        assertThat(prefs.autoBackupLedger.first()).isEmpty()
 
         prefs.setAutoBackupCombined(false)
         assertThat(prefs.autoBackupCombined.first()).isFalse()
@@ -765,6 +765,20 @@ class SettingsPrefsTest {
         assertThat(prefs.autoBackupKeep.first()).isEqualTo(10)
     }
 
+    @Test
+    fun `a pre-411 install adopts its old file when the layout is pinned in place`(@TempDir tempDir: Path) = runTest {
+        val store = testDataStore(tempDir, "test_settings")
+        val prefs = SettingsPrefs(store)
+        store.updateData { p ->
+            val m = p.toMutablePreferences()
+            m[stringPreferencesKey("auto_backup_folder_uri")] = "content://tree/old"
+            m
+        }
+        prefs.setAutoBackupCombined(false)
+        assertThat(prefs.autoBackupLedger.first())
+            .containsExactly("latest-combined", listOf("calendula-backup.ics"))
+    }
+
     private fun backupCalendar(id: Long, local: Boolean) = CalendarSource(
         id = id,
         displayName = "C$id",

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

@@ -132,7 +132,8 @@ class BackupPlanTest {
 
     @Test
     fun `the pre-411 file is the combined stable copy only for the install that wrote it`() {
-        val owned = planBackupFiles(listOf(combined), emptyMap(), setOf(LEGACY_BACKUP_FILE), at, 1, legacyOwned = true)
+        val adopted = mapOf(latestLedgerKey(COMBINED_SERIES) to listOf(LEGACY_BACKUP_FILE))
+        val owned = planBackupFiles(listOf(combined), adopted, setOf(LEGACY_BACKUP_FILE), at, 1)
         assertThat(owned.single().latestName).isEqualTo(LEGACY_BACKUP_FILE)
         assertThat(owned.single().prune).isEmpty()
 
@@ -144,7 +145,7 @@ class BackupPlanTest {
     fun `a stable name recorded as a dated file by an earlier build is never pruned`() {
         val ledger = mapOf(COMBINED_SERIES to listOf(LEGACY_BACKUP_FILE, "calendula-backup_a.ics"))
         val plan = planBackupFiles(
-            listOf(combined), ledger, setOf(LEGACY_BACKUP_FILE, "calendula-backup_a.ics"), at, 1, legacyOwned = true,
+            listOf(combined), ledger, setOf(LEGACY_BACKUP_FILE, "calendula-backup_a.ics"), at, 1,
         ).single()
         assertThat(plan.prune).containsExactly("calendula-backup_a.ics")
     }
@@ -266,4 +267,13 @@ class BackupPlanTest {
         assertThat(targets.map { it.baseName }).containsExactly("Work-5-5", "Work-6", "Work-5-9").inOrder()
     }
 
+    @Test
+    fun `claiming a stable copy takes it off its previous owner`() {
+        val previous = mapOf(latestLedgerKey("1") to listOf("Work.ics", "Job.ics"))
+        val next = withLedgerEntry(previous, latestLedgerKey("2"), listOf("Work.ics"), setOf("1", "2"))
+        assertThat(next).containsExactly(
+            latestLedgerKey("1"), listOf("Job.ics"),
+            latestLedgerKey("2"), listOf("Work.ics"),
+        )
+    }
 }

docs/ARCHITECTURE.md

@@ -472,7 +472,8 @@ automatic backup writes one `<calendar name>_<date>_<time>.ics` series per
 calendar, or one `calendula-backup_…` series in combined mode (#411), and prunes
 each series to the retention count after its new file has landed. Pruning only
 deletes names recorded in a per-series ledger of files the backup wrote
-(`auto_backup_files`, keyed by calendar id), never whatever happens to match a
+(`auto_backup_files`, keyed by calendar id, reset when the folder changes),
+never whatever happens to match a
 name, so renames, same-named calendars and the user's own files are safe.
 Each series also keeps a stable undated copy (`<calendar name>.ics`, or
 `calendula-backup.ics` combined) overwritten every run, so sync tools and