From 09c20ee90f66dfe2227e27d65ef44955e2eb37d1 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 14 Nov 2026 12:19:24 +0100 Subject: [PATCH] fix(#516): review round 7 - re-arm queues behind a running slot, hashes only while the option is on, one series in memory at a time --- .../calendula/data/backup/AutoBackup.kt | 96 ++++++++++--------- .../calendula/data/prefs/SettingsPrefs.kt | 6 +- .../calendula/domain/backup/BackupPlan.kt | 2 +- docs/ARCHITECTURE.md | 6 +- 4 files changed, 61 insertions(+), 49 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/backup/AutoBackup.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/backup/AutoBackup.kt index 2a0d26dc..c529e15a 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/backup/AutoBackup.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/backup/AutoBackup.kt @@ -18,6 +18,7 @@ import androidx.work.ExistingPeriodicWorkPolicy import androidx.work.ExistingWorkPolicy import androidx.work.OneTimeWorkRequestBuilder import androidx.work.PeriodicWorkRequestBuilder +import androidx.work.WorkInfo import androidx.work.WorkManager import androidx.work.WorkerParameters import androidx.work.await @@ -146,11 +147,12 @@ object BackupScheduler { * 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 a change run arms the other slot as it starts, catching - * changes made during the run, and again as it ends in case that slot was - * still busy. A slot already waiting or running is kept, so while two - * change runs overlap nothing watches until the second one ends. + * changes made during the run, and again as it ends. A slot already + * waiting is kept; one still running gets the new watch queued behind it, + * so the watch can't be lost to a run that is just finishing. While two + * change runs overlap, nothing watches until the first one ends. */ - internal fun watchChanges(context: Context, slot: Int) { + internal suspend fun watchChanges(context: Context, slot: Int) { val constraints = Constraints.Builder() .addContentUriTrigger(CalendarContract.CONTENT_URI, true) .setTriggerContentUpdateDelay(CHANGE_SETTLE_MINUTES, TimeUnit.MINUTES) @@ -161,7 +163,15 @@ object BackupScheduler { .setConstraints(constraints) .setInputData(workDataOf(KEY_CHANGE_SLOT to slot)) .build() - WorkManager.getInstance(context).enqueueUniqueWork(CHANGE_SLOTS[slot], ExistingWorkPolicy.KEEP, request) + val workManager = WorkManager.getInstance(context) + val pending = workManager.getWorkInfosForUniqueWorkFlow(CHANGE_SLOTS[slot]).first() + .filterNot { it.state.isFinished } + val policy = if (pending.isNotEmpty() && pending.all { it.state == WorkInfo.State.RUNNING }) { + ExistingWorkPolicy.APPEND_OR_REPLACE + } else { + ExistingWorkPolicy.KEEP + } + workManager.enqueueUniqueWork(CHANGE_SLOTS[slot], policy, request) } } @@ -277,21 +287,17 @@ class BackupWorker( } fun fingerprintOf(target: BackupTarget, text: String) = backupFingerprint(text, latestFileName(target.baseName)) - // A change run reads first and leaves the folder alone when nothing moved. - // It keeps the text of a changed (or unreadable, null) series only. - val unchangedHashes = HashMap() - val changedTexts = HashMap() - if (latestOnly) { - for (target in targets) { - val text = export(target) - val hash = text?.let { fingerprintOf(target, it) } - if (hash != null && hash == fingerprints[target.key]) { - unchangedHashes[target.key] = hash - } else { - changedTexts[target.key] = text - } - } - if (changedTexts.isEmpty()) return RunOutcome.NothingToDo + // Hashes only serve change runs; without them a stale one could pass later. + val trackHashes = latestOnly || prefs.autoBackupOnChange.first() + if (!trackHashes && fingerprints.isNotEmpty()) prefs.updateAutoBackupFingerprints(folder.toString()) { emptyMap() } + // A change run hashes first (null: unreadable) and leaves the folder alone when nothing moved. + val preHashes: Map = if (latestOnly) { + targets.associate { target -> target.key to export(target)?.let { fingerprintOf(target, it) } } + } else { + emptyMap() + } + if (latestOnly && targets.all { preHashes[it.key].let { hash -> hash != null && hash == fingerprints[it.key] } }) { + return RunOutcome.NothingToDo } prefs.adoptLegacyBackupFile() val listing = exporter.listFolder(folder) @@ -327,7 +333,9 @@ class BackupWorker( var landed: String? = null return try { val latestKey = latestLedgerKey(target.key) - if (target.key in fingerprints) prefs.updateAutoBackupFingerprints(folder.toString()) { it - target.key } + if (trackHashes && target.key in fingerprints) { + prefs.updateAutoBackupFingerprints(folder.toString()) { it - target.key } + } landed = exporter.writeToFolder(folder, listing, latest, bytes) // A provider that renamed it leaves a copy to clean up next // run; the intended name stays ours only if it already was. @@ -337,14 +345,14 @@ class BackupWorker( suspend fun recordLatest(stale: List, landedFingerprint: String? = null) = prefs.updateAutoBackupLedger( folder.toString(), - fingerprints = { old -> - landedFingerprint?.let { withFingerprint(old, target.key, it, liveKeys) } ?: old + fingerprints = landedFingerprint?.let { hash -> + { old -> withFingerprint(old, target.key, hash, liveKeys) } }, ) { val pending = plan.handOff - it.latestHeldElsewhere(target.key) withLedgerEntry(it, latestKey, stale + pending + current, liveKeys) } - recordLatest(plan.staleLatest, fingerprint) + recordLatest(plan.staleLatest, fingerprint.takeIf { trackHashes }) val staleLeft = exporter.deleteFromFolder(listing, plan.staleLatest) recordLatest(plan.staleLatest.filter { it in staleLeft }) if (landed == latest) LatestWrite.Landed else LatestWrite.Renamed @@ -363,31 +371,31 @@ class BackupWorker( val landedLatest = mutableSetOf() for (plan in plans) { val target = plan.target - var text: String? = null - val fingerprint = unchangedHashes[target.key] ?: run { - text = (if (target.key in changedTexts) changedTexts.remove(target.key) else export(target)) - text?.let { fingerprintOf(target, it) } - } ?: run { - failed++ - continue - } - if (plan.latestName == null) { - // Nothing to refresh until the content moves, so a later change run can stop early. - if (fingerprints[target.key] != fingerprint) { - prefs.updateAutoBackupFingerprints(folder.toString()) { - withFingerprint(it, target.key, fingerprint, liveKeys) - } + // A copy blocked by a file of the user's keeps its hash, so a later change run can stop early. + suspend fun rememberBlocked(hash: String) { + if (plan.latestName == null && trackHashes && fingerprints[target.key] != hash) { + prefs.updateAutoBackupFingerprints(folder.toString()) { withFingerprint(it, target.key, hash, liveKeys) } } - if (latestOnly) continue } - if (latestOnly && !plan.needsRefresh(fingerprint, fingerprints, listing.files.keys, listing.complete)) { - if (plan.latestName in listing.files) landedLatest += target.key - continue + if (latestOnly) { + val hash = preHashes[target.key] ?: run { + failed++ + continue + } + rememberBlocked(hash) + if (!plan.needsRefresh(hash, fingerprints, listing.files.keys, listing.complete)) { + if (plan.latestName in listing.files) landedLatest += target.key + continue + } } - val bytes = (text ?: export(target) ?: run { + // Read again rather than held since the pre-pass: one series' text in memory at a time. + val text = export(target) ?: run { failed++ continue - }).toByteArray(Charsets.UTF_8) + } + val fingerprint = fingerprintOf(target, text) + rememberBlocked(fingerprint) + val bytes = text.toByteArray(Charsets.UTF_8) // Not cancellable once a file exists: it must reach the ledger. withContext(NonCancellable) { val dated = if (latestOnly) null else writeDated(plan, bytes) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefs.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefs.kt index 3101b74e..74b0bce4 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefs.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/SettingsPrefs.kt @@ -387,13 +387,15 @@ class SettingsPrefs @Inject constructor( */ suspend fun updateAutoBackupLedger( folder: String, - fingerprints: (BackupFingerprints) -> BackupFingerprints = { it }, + fingerprints: ((BackupFingerprints) -> BackupFingerprints)? = null, transform: (BackupLedger) -> BackupLedger, ) { prefs.edit { if (it[SettingsKeys.autoBackupFolderUri] == folder) { it[SettingsKeys.autoBackupLedger] = transform(it[SettingsKeys.autoBackupLedger]) - it[SettingsKeys.autoBackupFingerprints] = fingerprints(it[SettingsKeys.autoBackupFingerprints]) + if (fingerprints != null) { + it[SettingsKeys.autoBackupFingerprints] = fingerprints(it[SettingsKeys.autoBackupFingerprints]) + } } } } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/domain/backup/BackupPlan.kt b/app/src/main/java/de/jeanlucmakiola/calendula/domain/backup/BackupPlan.kt index 1b536785..76a6f269 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/domain/backup/BackupPlan.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/domain/backup/BackupPlan.kt @@ -357,7 +357,7 @@ fun List.inBackupOrder(): List = sortedWith( compareBy( { it.overrideOf == null }, { it.start }, { it.uid }, { it.overrideAt }, { it.calendarName }, - { it.end }, { it.summary }, { it.description }, { it.location }, + { it.toString() }, ), ) .map { diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b8e5577a..7070d602 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -491,7 +491,8 @@ 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 again as it ends. Runs share the +(catching changes made during the run) and again as it ends, queued behind +that slot's run if it is still running. 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 @@ -499,7 +500,8 @@ 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 a change run +`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