fix: harden backup restore and alarm playback

CODE_QUALITY_REVIEW.md

@@ -0,0 +1,97 @@
+# Code quality review
+
+Reviewed on 2026-10-08. Scope: tracked app source, tests, Gradle and release configuration, scripts, and the checked-out `floret-kit` submodule. This is a source review, so severity describes the consequence of the visible failure path, not a measured production incident. Generated build output and translation content were excluded.
+
+**Implementation update (2026-10-08):** The eight findings below describe the code as reviewed. The working tree now includes hardening for each: staged SAF export, bounded import, transactional settings restore with database-failure compensation, guaranteed backup UI cleanup, audio-start fallback, media-browser cleanup, user-volume preservation, and failure-path tests. The backup still spans Room and DataStore, which cannot share a single transaction; a failure after the Room commit can leave the imported data in place while platform reconciliation reports an error.
+
+## Summary
+
+The project has clear domain boundaries, transactional DAO helpers, explicit clock abstractions, and broad unit-test coverage. The highest-risk gaps are in backup replacement and alarm audio fallback. Each finding includes a severity and points to the lines that establish the issue; no source is reproduced here.
+
+## Findings
+
+### 1. Existing backup is deleted before a replacement is safely written — High
+
+**Location:** `app/src/main/java/de/jeanlucmakiola/clockula/backup/android/SafBackupFiles.kt:28-37`
+
+**Principle broken:** Preserve the last known good copy until the replacement succeeds; make destructive file updates atomic where the storage provider allows it.
+
+**Impact:** Export removes an existing backup before creating or writing the new document. If creation, stream opening, writing, or provider storage fails, the method returns a failure but the previous backup is already gone. The return value from `delete()` is also ignored, so a failed delete can lead to provider-dependent duplicate naming or a second failure.
+
+**Recommendation:** Write to a temporary sibling document first, close and verify it, then replace the old document. Where SAF providers cannot guarantee atomic rename, keep the old backup until the new one is complete and report cleanup failures separately. Test provider failures at each step.
+
+### 2. Backup import can leave live state partially replaced — High
+
+**Locations:** `app/src/main/java/de/jeanlucmakiola/clockula/backup/BackupEngine.kt:51-60,69-80`; `app/src/main/java/de/jeanlucmakiola/clockula/data/backup/BackupDao.kt:44-56`
+
+**Principle broken:** A user-visible restore should either complete or recover to a coherent previous state.
+
+**Impact:** Room makes the three table replacements atomic with each other, but the engine first dismisses a ringing alarm and deletes running timers, then replaces the tables, then writes settings through multiple independent DataStore edits. A failure or cancellation after any of these steps can leave old data altered, new tables paired with old or partially imported settings, and schedulers out of sync. The DAO transaction does not cover the rest of the restore.
+
+**Recommendation:** Stage and validate the full import before touching live state. Define a recoverable commit protocol across Room and DataStore, or use a compensating snapshot and rollback. Ensure scheduler reconciliation runs in a `finally` block after any committed change. Add fault-injection tests for each stage.
+
+### 3. Backup failures outside SAF can crash the screen and leave it busy — High
+
+**Location:** `app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/BackupViewModel.kt:39-73`
+
+**Principle broken:** Boundaries for user-initiated I/O should turn expected failures into UI state and always clear progress state.
+
+**Impact:** The view model folds failures returned by `BackupFiles`, but `engine.export()`, codec encoding/decoding, and `engine.import()` can throw. In those cases the coroutine exits before resetting `_busy` or sending a failure event. An import failure can follow the partial writes described above. The existing tests cover successful restores, not these exceptions.
+
+**Recommendation:** Catch non-cancellation failures around the entire operation, map them to `BackupEvent.*Failed`, and reset `_busy` in `finally`. Preserve coroutine cancellation. Add tests for export, decode, and restore failures.
+
+### 4. A picked backup file is read into memory without a size limit — Medium
+
+**Locations:** `app/src/main/java/de/jeanlucmakiola/clockula/backup/android/SafBackupFiles.kt:41-46`; `app/src/main/java/de/jeanlucmakiola/clockula/data/backup/BackupCodec.kt:43-51,116-180`
+
+**Principle broken:** Bound untrusted input before allocating or parsing it.
+
+**Impact:** The document picker accepts an arbitrary provider document, and `readBytes()` holds the entire file in memory before JSON decoding builds further lists. A very large or malformed file can exhaust memory or stall the import UI before the codec can reject it. The codec also has no count limits for alarms, timers, or world clocks.
+
+**Recommendation:** Set a documented maximum byte count during a buffered read, reject oversized documents before decoding, and validate collection counts and string lengths. Keep the limits above any realistic export size.
+
+### 5. Alarm playback reports success even if `MediaPlayer.start()` fails — High
+
+**Location:** `app/src/main/java/de/jeanlucmakiola/clockula/ring/RingAudioPlayer.kt:149-159`
+
+**Principle broken:** A fallible operation must determine the success result before the caller suppresses its fallback.
+
+**Impact:** `start()` is wrapped in `runCatching`, but its result is discarded and `startPlayer()` returns `true` regardless. When a prepared player fails to start, the outer ring path considers audio audible and does not call `onSilenced()` to enable forced vibration. The player and focus request also remain held until a later stop. This can produce a silent alarm or timer.
+
+**Recommendation:** Return failure when `start()` throws, release the prepared player and audio focus, and let the normal fallback path call `onSilenced()`. Cover this with an injectable player seam and a start-failure test.
+
+### 6. Failed media-browser connections are not disconnected — Medium
+
+**Location:** `app/src/main/java/de/jeanlucmakiola/clockula/data/ringtones/MediaBrowserConnect.kt:24-45`
+
+**Principle broken:** Every acquired resource needs cleanup on all completion paths.
+
+**Impact:** `onConnectionFailed()` and pre-connection `onConnectionSuspended()` resume with `null` without disconnecting the created browser. The cancellation hook runs only on cancellation, while the caller receives no browser to release on these ordinary failure paths. Repeated failed music-app connections can retain browser bindings and related resources.
+
+**Recommendation:** Disconnect in both failure callbacks before resuming. Make completion and cleanup idempotent because callbacks and cancellation can race. Add callback-order tests.
+
+### 7. Ring cleanup can overwrite a user's media-volume change — Medium
+
+**Locations:** `app/src/main/java/de/jeanlucmakiola/clockula/ring/MediaRingRegistry.kt:21-40`; `app/src/main/java/de/jeanlucmakiola/clockula/ring/MediaAppRing.kt:113-119`
+
+**Principle broken:** Restore temporary state only when it is still owned by the operation; do not overwrite a concurrent user change.
+
+**Impact:** The registry saves the music volume at ring start and unconditionally restores it when the last ring ends. If the user changes media volume during the ring, dismissal puts it back to the old value. This is especially surprising when the user lowers the volume to silence the music app.
+
+**Recommendation:** Track the last volume value written by the ring and restore the saved value only if the current value still matches it, or observe volume changes and treat them as user-owned. Test changes both above and below the ring level.
+
+### 8. Backup tests do not exercise failed replacement or restore — Medium
+
+**Locations:** `app/src/test/java/de/jeanlucmakiola/clockula/backup/BackupEngineTest.kt:49-70,88-111,114-160`; `app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/BackupViewModel.kt:39-73`
+
+**Principle broken:** Destructive workflows need tests for failure boundaries as well as successful round trips.
+
+**Impact:** The visible engine tests verify successful import, ring dismissal, timer deletion, and scheduling. They do not inject failures between the destructive steps or test whether the UI reports an error and clears busy state. The most consequential restore paths are therefore unguarded by regression tests.
+
+**Recommendation:** Add failure-injection tests for repository replacement, each settings write stage, engine rescheduling, and SAF replacement. Assert both final stored state and user-visible error state.
+
+## Verification and limits
+
+- `bash -n` passed for tracked shell scripts; `python3 -m py_compile scripts/*.py` passed.
+- During the original review, Gradle could not start inside the restricted cache sandbox. After the hardening changes, the offline `:app:testDebugUnitTest :app:lintDebug` run completed successfully with 1,596 tests, zero failures, and zero errors.
+- Static review cannot establish how often platform providers throw or fail. Findings 1, 3, 5, and 6 follow directly from unchecked return or exception paths; findings 2, 4, and 7 describe reproducible state or resource risks under their stated conditions.

app/src/main/java/de/jeanlucmakiola/clockula/alarm/AlarmEngine.kt

@@ -157,6 +157,13 @@ class AlarmEngine @Inject constructor(
         )
     }
 
+    /** Silence the current service without changing its persisted ring state. */
+    suspend fun suspendRingForImport(): Unit = lock.withLock {
+        val alarmId = states.ringing()?.alarmId ?: return@withLock
+        ring.stopRinging(alarmId)
+        scheduler.cancelAutoSilence()
+    }
+
     /**
      * Read-only. Never writes. Sorted by nextFire ascending, nulls last, then by id.
      *

app/src/main/java/de/jeanlucmakiola/clockula/backup/BackupEngine.kt

@@ -3,13 +3,13 @@ package de.jeanlucmakiola.clockula.backup
 import de.jeanlucmakiola.clockula.alarm.AlarmEngine
 import de.jeanlucmakiola.clockula.data.backup.BackupRepository
 import de.jeanlucmakiola.clockula.data.prefs.SettingsPrefs
-import de.jeanlucmakiola.clockula.data.timers.TimerRepository
-import de.jeanlucmakiola.clockula.domain.TimerState
 import de.jeanlucmakiola.clockula.domain.backup.BackupContent
 import de.jeanlucmakiola.clockula.domain.backup.BackupSettings
 import de.jeanlucmakiola.clockula.domain.time.WallClock
 import de.jeanlucmakiola.clockula.timer.TimerEngine
+import kotlinx.coroutines.NonCancellable
 import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.withContext
 import javax.inject.Inject
 import javax.inject.Singleton
 
@@ -24,7 +24,6 @@ import javax.inject.Singleton
 class BackupEngine @Inject constructor(
     private val repository: BackupRepository,
     private val settings: SettingsPrefs,
-    private val timers: TimerRepository,
     private val alarmEngine: AlarmEngine,
     private val timerEngine: TimerEngine,
     private val wallClock: WallClock,
@@ -39,24 +38,64 @@ class BackupEngine @Inject constructor(
         settings = settingsSnapshot(),
     )
 
-    /**
-     * Replaces every alarm, timer and world clock, and every setting the
-     * backup carries. Before the replace: a ringing alarm is dismissed (the
-     * same `stopRingIfRinging` step `AlarmsViewModel` takes before disabling
-     * one) and every running timer is deleted through [TimerEngine], so the
-     * import can never strand a live ring session mid-replace. After it:
-     * both engines are told to re-resolve, so the imported state is live
-     * immediately rather than waiting for the next unrelated trigger.
-     */
+    /** Replace settings and rows, then reconcile the platform services. */
     suspend fun import(content: BackupContent) {
-        stopRingingAlarmIfAny()
-        deleteRunningTimers()
-
-        repository.replaceAll(content.alarms, content.timers, content.worldClocks)
-        applySettings(content.settings)
-
-        alarmEngine.reschedule()
-        timerEngine.resync()
+        val previousSettings = settingsSnapshot()
+        var settingsApplied = false
+        var rowsReplaced = false
+        var ringSuspended = false
+        var failure: Throwable? = null
+        try {
+            withContext(NonCancellable) {
+                settings.applyBackup(content.settings)
+                settingsApplied = true
+            }
+            // Pause actuators without deleting their rows. If Room rejects the
+            // replacement, the old ring and running timers can be resumed.
+            alarmEngine.suspendRingForImport()
+            ringSuspended = true
+            timerEngine.suspendForImport()
+            withContext(NonCancellable) {
+                repository.replaceAll(content.alarms, content.timers, content.worldClocks)
+                rowsReplaced = true
+            }
+        } catch (caught: Throwable) {
+            failure = caught
+        } finally {
+            withContext(NonCancellable) {
+                if (!rowsReplaced && settingsApplied) {
+                    try {
+                        settings.applyBackup(previousSettings)
+                    } catch (rollbackFailure: Throwable) {
+                        failure?.addSuppressed(rollbackFailure)
+                    }
+                }
+                // The database transaction either committed in full or left its
+                // old rows. In both cases refresh every platform registration.
+                try {
+                    if (!rowsReplaced && ringSuspended) alarmEngine.resumeRingingIfAny()
+                    alarmEngine.reschedule()
+                } catch (reconcileFailure: Throwable) {
+                    val reported = if (rowsReplaced) {
+                        IllegalStateException("backup imported, but alarm scheduling could not be refreshed", reconcileFailure)
+                    } else {
+                        reconcileFailure
+                    }
+                    if (failure == null) failure = reported else failure?.addSuppressed(reported)
+                }
+                try {
+                    timerEngine.resync()
+                } catch (reconcileFailure: Throwable) {
+                    val reported = if (rowsReplaced) {
+                        IllegalStateException("backup imported, but timer scheduling could not be refreshed", reconcileFailure)
+                    } else {
+                        reconcileFailure
+                    }
+                    if (failure == null) failure = reported else failure?.addSuppressed(reported)
+                }
+            }
+        }
+        failure?.let { throw it }
     }
 
     private suspend fun settingsSnapshot(): BackupSettings = BackupSettings(
@@ -65,28 +104,4 @@ class BackupEngine @Inject constructor(
         defaults = settings.currentDefaults(),
         timerPresets = settings.timerPresets.first(),
     )
-
-    private suspend fun applySettings(backup: BackupSettings) {
-        settings.setThemeMode(backup.themeMode)
-        settings.setDynamicColor(backup.dynamicColor)
-        settings.setSnoozeMinutes(backup.defaults.snoozeMinutes)
-        settings.setSnoozeLimit(backup.defaults.snoozeLimit)
-        settings.setVibrate(backup.defaults.vibrate)
-        settings.setVolumeRampSeconds(backup.defaults.volumeRampSeconds)
-        settings.setAlarmRingtoneUri(backup.defaults.alarmRingtoneUri)
-        settings.setTimerRingtoneUri(backup.defaults.timerRingtoneUri)
-        settings.setDismissChallenge(backup.defaults.dismissChallenge)
-        settings.setHomeZoneId(backup.defaults.homeZoneId)
-        settings.setTimerPresets(backup.timerPresets)
-    }
-
-    private suspend fun stopRingingAlarmIfAny() {
-        val ringingId = alarmEngine.ringSession()?.alarm?.id ?: return
-        alarmEngine.dismiss(ringingId)
-    }
-
-    private suspend fun deleteRunningTimers() {
-        val runningIds = timers.timers().first().filter { it.state == TimerState.RUNNING }.map { it.id }
-        for (id in runningIds) timerEngine.delete(id)
-    }
 }

app/src/main/java/de/jeanlucmakiola/clockula/backup/android/SafBackupFiles.kt

@@ -9,7 +9,9 @@ import de.jeanlucmakiola.clockula.backup.BackupFiles
 import de.jeanlucmakiola.floret.di.IoDispatcher
 import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.withContext
+import java.io.ByteArrayOutputStream
 import java.io.IOException
+import java.util.UUID
 import javax.inject.Inject
 import javax.inject.Singleton
 
@@ -27,14 +29,29 @@ class SafBackupFiles @Inject constructor(
 
     override suspend fun write(treeUri: String, content: String): Result<Unit> = withContext(io) {
         runCatching {
+            val bytes = content.toByteArray(Charsets.UTF_8)
+            if (bytes.size > MAX_BACKUP_BYTES) throw IOException("backup exceeds the 8 MiB limit")
             val tree = DocumentFile.fromTreeUri(context, treeUri.toUri())
                 ?: error("could not open the chosen folder")
-            tree.findFile(BACKUP_FILE_NAME)?.delete()
-            val file = tree.createFile("application/json", BACKUP_FILE_NAME)
-                ?: error("could not create $BACKUP_FILE_NAME")
-            val stream = context.contentResolver.openOutputStream(file.uri)
-                ?: throw IOException("could not open an output stream for ${file.uri}")
-            stream.use { it.write(content.toByteArray(Charsets.UTF_8)) }
+            val temporary = tree.createFile("application/json", "$BACKUP_FILE_NAME.${UUID.randomUUID()}.tmp")
+                ?: throw IOException("could not create a temporary backup")
+            var keepTemporary = false
+            try {
+                val stream = context.contentResolver.openOutputStream(temporary.uri)
+                    ?: throw IOException("could not open an output stream for ${temporary.uri}")
+                stream.use { it.write(bytes) }
+                val existing = tree.findFile(BACKUP_FILE_NAME)
+                if (existing != null && !existing.delete()) throw IOException("could not remove the previous backup")
+                // Some SAF providers cannot rename. Once the old file has been
+                // removed, retain the complete temporary file if rename fails.
+                keepTemporary = true
+                if (!temporary.renameTo(BACKUP_FILE_NAME)) {
+                    throw IOException("backup was written as ${temporary.name}; the provider could not rename it")
+                }
+            } catch (failure: Throwable) {
+                if (!keepTemporary) runCatching { temporary.delete() }
+                throw failure
+            }
         }
     }
 
@@ -42,7 +59,23 @@ class SafBackupFiles @Inject constructor(
         runCatching {
             val stream = context.contentResolver.openInputStream(documentUri.toUri())
                 ?: throw IOException("could not open an input stream for $documentUri")
-            stream.use { it.readBytes().toString(Charsets.UTF_8) }
+            stream.use { input ->
+                val output = ByteArrayOutputStream()
+                val buffer = ByteArray(8192)
+                var total = 0
+                while (true) {
+                    val count = input.read(buffer)
+                    if (count < 0) break
+                    total += count
+                    if (total > MAX_BACKUP_BYTES) throw IOException("backup exceeds the 8 MiB limit")
+                    output.write(buffer, 0, count)
+                }
+                output.toString(Charsets.UTF_8.name())
+            }
         }
     }
+
+    private companion object {
+        const val MAX_BACKUP_BYTES = 8 * 1024 * 1024
+    }
 }

app/src/main/java/de/jeanlucmakiola/clockula/data/backup/BackupCodec.kt

@@ -32,6 +32,13 @@ sealed interface BackupResult {
  */
 object BackupCodec {
 
+    private const val MAX_DOCUMENT_CHARS = 8 * 1024 * 1024
+    private const val MAX_ALARMS = 10_000
+    private const val MAX_TIMERS = 10_000
+    private const val MAX_WORLD_CLOCKS = 1_000
+    private const val MAX_LABEL_CHARS = 64 * 1_024
+    private const val MAX_URI_CHARS = 64 * 1_024
+
     private val json = Json {
         ignoreUnknownKeys = true
         prettyPrint = true
@@ -41,6 +48,7 @@ object BackupCodec {
         json.encodeToString(BackupDocument.serializer(), content.toDocument())
 
     fun decode(text: String): BackupResult {
+        if (text.length > MAX_DOCUMENT_CHARS) return BackupResult.Failure("backup exceeds the 8 MiB limit")
         val document = try {
             json.decodeFromString(BackupDocument.serializer(), text)
         } catch (e: SerializationException) {
@@ -112,6 +120,9 @@ object BackupCodec {
     private fun BackupDocument.toContent(): BackupResult {
         if (format != BACKUP_FORMAT) return BackupResult.Failure("unknown format: '$format'")
         if (version != BACKUP_VERSION) return BackupResult.Failure("unsupported backup version: $version")
+        if (alarms.size > MAX_ALARMS || timers.size > MAX_TIMERS || worldClocks.size > MAX_WORLD_CLOCKS) {
+            return BackupResult.Failure("backup contains too many records")
+        }
 
         val alarms = mutableListOf<BackupAlarm>()
         for ((index, dto) in this.alarms.withIndex()) {
@@ -120,6 +131,9 @@ object BackupCodec {
                     "alarms[$index]: hour/minute out of range (${dto.hour}:${dto.minute})",
                 )
             }
+            if (dto.label.length > MAX_LABEL_CHARS || (dto.ringtoneUri?.length ?: 0) > MAX_URI_CHARS) {
+                return BackupResult.Failure("alarms[$index]: label or ringtone URI is too long")
+            }
             alarms += BackupAlarm(
                 time = TimeOfDay(dto.hour, dto.minute),
                 label = dto.label,
@@ -141,6 +155,9 @@ object BackupCodec {
             if (dto.durationMillis < 0) {
                 return BackupResult.Failure("timers[$index]: negative duration")
             }
+            if (dto.label.length > MAX_LABEL_CHARS || (dto.ringtoneUri?.length ?: 0) > MAX_URI_CHARS) {
+                return BackupResult.Failure("timers[$index]: label or ringtone URI is too long")
+            }
             timers += BackupTimer(
                 label = dto.label,
                 duration = dto.durationMillis.milliseconds,
@@ -155,6 +172,9 @@ object BackupCodec {
             if (!Zones.isValid(dto.zoneId)) {
                 return BackupResult.Failure("worldClocks[$index]: not an IANA zone id: '${dto.zoneId}'")
             }
+            if ((dto.label?.length ?: 0) > MAX_LABEL_CHARS) {
+                return BackupResult.Failure("worldClocks[$index]: label is too long")
+            }
             worldClocks += BackupWorldClock(
                 zoneId = dto.zoneId,
                 label = dto.label?.takeIf { it.isNotBlank() },
@@ -163,6 +183,12 @@ object BackupCodec {
         }
 
         val settingsDto = settings
+        if ((settingsDto.alarmRingtoneUri?.length ?: 0) > MAX_URI_CHARS ||
+            (settingsDto.timerRingtoneUri?.length ?: 0) > MAX_URI_CHARS ||
+            settingsDto.timerPresets.size > 1_000
+        ) {
+            return BackupResult.Failure("settings contain oversized values")
+        }
         val settingsContent = BackupSettings(
             themeMode = settingsDto.themeMode.toEnum(ThemeMode.SYSTEM),
             dynamicColor = settingsDto.dynamicColor,

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

@@ -1,7 +1,13 @@
 package de.jeanlucmakiola.clockula.data.prefs
 
+import androidx.datastore.preferences.core.MutablePreferences
+import androidx.datastore.preferences.core.booleanPreferencesKey
+import androidx.datastore.preferences.core.intPreferencesKey
+import androidx.datastore.preferences.core.stringPreferencesKey
 import de.jeanlucmakiola.clockula.domain.ClockDefaults
 import de.jeanlucmakiola.clockula.domain.DismissChallenge
+import de.jeanlucmakiola.clockula.domain.Zones
+import de.jeanlucmakiola.clockula.domain.backup.BackupSettings
 import de.jeanlucmakiola.clockula.domain.timer.TimerPresets
 import de.jeanlucmakiola.floret.prefs.Appearance
 import de.jeanlucmakiola.floret.prefs.AppearancePrefs
@@ -73,6 +79,31 @@ class SettingsPrefs @Inject constructor(private val store: PrefStore) {
 
     suspend fun currentDefaults(): ClockDefaults = defaults.first()
 
+    /** Restore every backed-up preference in one DataStore transaction. */
+    suspend fun applyBackup(backup: BackupSettings) {
+        store.edit { prefs ->
+            // PrefStore's multi-key edit is public, while Pref.write belongs to
+            // floret-kit. Use the same named keys and normalisation as the
+            // individual ClockPrefs/TimerPrefs setters in this one transaction.
+            prefs[stringPreferencesKey(AppearancePrefs.themeMode.name)] = backup.themeMode.name
+            prefs[booleanPreferencesKey(AppearancePrefs.dynamicColor.name)] = backup.dynamicColor
+            prefs[intPreferencesKey(ClockPrefs.snoozeMinutes.name)] = backup.defaults.snoozeMinutes.coerceIn(1..60)
+            prefs[intPreferencesKey(ClockPrefs.snoozeLimit.name)] = backup.defaults.snoozeLimit.coerceIn(0..10)
+            prefs[booleanPreferencesKey(ClockPrefs.vibrate.name)] = backup.defaults.vibrate
+            prefs[intPreferencesKey(ClockPrefs.volumeRampSeconds.name)] = backup.defaults.volumeRampSeconds.coerceIn(0..60)
+            prefs.setNullable(ClockPrefs.alarmRingtoneUri.name, backup.defaults.alarmRingtoneUri?.trim()?.takeIf(String::isNotEmpty))
+            prefs.setNullable(ClockPrefs.timerRingtoneUri.name, backup.defaults.timerRingtoneUri?.trim()?.takeIf(String::isNotEmpty))
+            prefs[stringPreferencesKey(ClockPrefs.dismissChallenge.name)] = backup.defaults.dismissChallenge.name
+            prefs.setNullable(ClockPrefs.homeZoneId.name, backup.defaults.homeZoneId?.let(Zones::normalise))
+            prefs[stringPreferencesKey(TimerPrefs.presets.name)] = TimerPresets.encode(TimerPresets.sanitise(backup.timerPresets))
+        }
+    }
+
+    private fun MutablePreferences.setNullable(name: String, value: String?) {
+        val key = stringPreferencesKey(name)
+        if (value == null) remove(key) else this[key] = value
+    }
+
     suspend fun setSnoozeMinutes(minutes: Int) = store.set(ClockPrefs.snoozeMinutes, minutes)
 
     suspend fun setSnoozeLimit(limit: Int) = store.set(ClockPrefs.snoozeLimit, limit)

app/src/main/java/de/jeanlucmakiola/clockula/data/ringtones/MediaBrowserConnect.kt

@@ -21,25 +21,40 @@ internal suspend fun connectMediaBrowser(
     onSuspended: () -> Unit = {},
 ): MediaBrowser? = suspendCancellableCoroutine { continuation ->
     var created: MediaBrowser? = null
+    var handedOff = false
+    fun disconnect() {
+        created?.let { runCatching { it.disconnect() } }
+    }
     val callback = object : MediaBrowser.ConnectionCallback() {
         override fun onConnected() {
-            if (continuation.isActive) continuation.resume(created)
+            if (continuation.isActive) {
+                handedOff = true
+                continuation.resume(created)
+            } else {
+                disconnect()
+            }
         }
 
         override fun onConnectionFailed() {
+            disconnect()
             if (continuation.isActive) continuation.resume(null)
         }
 
         override fun onConnectionSuspended() {
-            if (continuation.isActive) continuation.resume(null) else onSuspended()
+            if (handedOff) {
+                onSuspended()
+            } else {
+                disconnect()
+                if (continuation.isActive) continuation.resume(null)
+            }
         }
     }
     val connecting = runCatching { MediaBrowser(context, component, callback, null) }.getOrNull()
     created = connecting
     if (connecting == null || runCatching { connecting.connect() }.isFailure) {
-        connecting?.let { runCatching { it.disconnect() } }
+        disconnect()
         continuation.resume(null)
         return@suspendCancellableCoroutine
     }
-    continuation.invokeOnCancellation { runCatching { connecting.disconnect() } }
+    continuation.invokeOnCancellation { disconnect() }
 }

app/src/main/java/de/jeanlucmakiola/clockula/ring/MediaAppRing.kt

@@ -37,9 +37,6 @@ internal class MediaAppRing(private val context: Context, private val ref: Media
     /** The fully ramped media index, fixed when the ring starts. */
     private var target: Int = 0
 
-    /** The index last written, so an unchanged ramp step writes nothing. */
-    private var lastIndex: Int? = null
-
     private var holding: Boolean = false
 
     /** The app's service went away mid-ring: nothing it reports any more is trustworthy. */
@@ -114,9 +111,7 @@ internal class MediaAppRing(private val context: Context, private val ref: Media
     fun setLevel(level: Float) {
         if (!holding) return
         val index = MediaRingPolicy.mediaVolumeIndex(level, target)
-        if (index == lastIndex) return
-        lastIndex = index
-        runCatching { audioManager.setStreamVolume(AudioManager.STREAM_MUSIC, index, 0) }
+        MediaRingRegistry.setLevel(index, audioManager)
     }
 
     /** Pauses the app (unless another ring still uses it), disconnects, releases the volume. Idempotent. */

app/src/main/java/de/jeanlucmakiola/clockula/ring/MediaRingRegistry.kt

@@ -14,6 +14,8 @@ internal object MediaRingRegistry {
 
     /** The user's media volume from before the first live media ring; null while none is live. */
     private var savedMusicVolume: Int? = null
+    private var lastRingVolume: Int? = null
+    private var userChangedVolume: Boolean = false
 
     /** Live media rings per package. */
     private val holders = mutableMapOf<String, Int>()
@@ -21,10 +23,29 @@ internal object MediaRingRegistry {
     fun acquire(packageName: String, audioManager: AudioManager) {
         if (holders.isEmpty()) {
             savedMusicVolume = runCatching { audioManager.getStreamVolume(AudioManager.STREAM_MUSIC) }.getOrNull()
+            lastRingVolume = null
+            userChangedVolume = false
         }
         holders[packageName] = (holders[packageName] ?: 0) + 1
     }
 
+    /** Stop controlling the stream as soon as another actor changes it. */
+    fun setLevel(index: Int, audioManager: AudioManager) {
+        if (userChangedVolume) return
+        val current = runCatching { audioManager.getStreamVolume(AudioManager.STREAM_MUSIC) }.getOrNull() ?: return
+        if (lastRingVolume != null && current != lastRingVolume) {
+            userChangedVolume = true
+            return
+        }
+        if (current == index) {
+            lastRingVolume = index
+            return
+        }
+        if (runCatching { audioManager.setStreamVolume(AudioManager.STREAM_MUSIC, index, 0) }.isSuccess) {
+            lastRingVolume = index
+        }
+    }
+
     /**
      * Drops one hold on [packageName]. True when no other live ring still uses
      * that app, so it is the caller's to pause. The user's media volume comes
@@ -34,10 +55,15 @@ internal object MediaRingRegistry {
         val count = holders[packageName] ?: return false
         if (count > 1) holders[packageName] = count - 1 else holders.remove(packageName)
         if (holders.isEmpty()) {
-            savedMusicVolume?.let { saved ->
-                runCatching { audioManager.setStreamVolume(AudioManager.STREAM_MUSIC, saved, 0) }
+            val current = runCatching { audioManager.getStreamVolume(AudioManager.STREAM_MUSIC) }.getOrNull()
+            if (!userChangedVolume && current != null && current == lastRingVolume) {
+                savedMusicVolume?.let { saved ->
+                    runCatching { audioManager.setStreamVolume(AudioManager.STREAM_MUSIC, saved, 0) }
+                }
             }
             savedMusicVolume = null
+            lastRingVolume = null
+            userChangedVolume = false
         }
         return packageName !in holders
     }

app/src/main/java/de/jeanlucmakiola/clockula/ring/RingAudioPlayer.kt

@@ -14,6 +14,7 @@ import de.jeanlucmakiola.clockula.domain.ring.RingAudio
 import de.jeanlucmakiola.clockula.domain.ring.RingtoneSourceKind
 import de.jeanlucmakiola.clockula.domain.ring.VolumeRamp
 import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.delay
@@ -115,7 +116,15 @@ class RingAudioPlayer(private val context: Context) {
         if (!routes.mediaAudible()) return false
         val media = MediaAppRing(context, ref)
         mediaRing = media
-        if (media.start() && ring == generation) {
+        val started = try {
+            media.start()
+        } catch (cancelled: CancellationException) {
+            media.stop()
+            throw cancelled
+        } catch (_: Exception) {
+            false
+        }
+        if (started && ring == generation) {
             watchMedia(ring, scope, media, ringtoneUri, rampFor, onSilenced)
             return true
         }
@@ -136,26 +145,29 @@ class RingAudioPlayer(private val context: Context) {
         rampFor: Duration,
     ): Boolean {
         if (ring != generation) return false
-        val prepared = withContext(Dispatchers.IO) {
-            sources(ringtoneUri).firstNotNullOfOrNull { prepare(it) }
-        } ?: return false
-
-        // Stopped while we were preparing: release rather than start, or the ring
-        // would outlive the dismissal that asked for it.
-        if (ring != generation) {
-            release(prepared)
-            return false
+        for (uri in sources(ringtoneUri)) {
+            val prepared = withContext(Dispatchers.IO) { prepare(uri) } ?: continue
+            // Stopped while preparing: never start an obsolete ring.
+            if (ring != generation) {
+                release(prepared)
+                return false
+            }
+            requestFocus()
+            val initial = VolumeRamp.levelAt(Duration.ZERO, rampFor)
+            val started = runCatching {
+                prepared.setVolume(initial, initial)
+                prepared.start()
+            }.isSuccess
+            if (!started) {
+                release(prepared)
+                abandonFocus()
+                continue
+            }
+            player = prepared
+            ramp = launchRamp(scope, rampFor) { level -> runCatching { player?.setVolume(level, level) } }
+            return true
         }
-        player = prepared
-
-        requestFocus()
-        // The level is set before the first sample plays, so a ramp from silence
-        // does not begin with a burst at full volume.
-        val initial = VolumeRamp.levelAt(Duration.ZERO, rampFor)
-        runCatching { prepared.setVolume(initial, initial) }
-        runCatching { prepared.start() }
-        ramp = launchRamp(scope, rampFor) { level -> runCatching { player?.setVolume(level, level) } }
-        return true
+        return false
     }
 
     /**

app/src/main/java/de/jeanlucmakiola/clockula/timer/TimerEngine.kt

@@ -69,6 +69,12 @@ class TimerEngine @Inject constructor(
     /** Sweep what is due, re-register the earliest deadline, match the service to "anything active". */
     suspend fun resync(): Duration? = lock.withLock { resyncLocked() }
 
+    /** Stop the current service and slot while a backup replaces its rows. */
+    suspend fun suspendForImport(): Unit = lock.withLock {
+        scheduler.cancelExpiry()
+        service.sync(false)
+    }
+
     /**
      * The slot arriving. The sweep is idempotent and carries no id (D5): a fire
      * delivered late expires everything that came due while it was delayed, and

app/src/main/java/de/jeanlucmakiola/clockula/ui/settings/BackupViewModel.kt

@@ -8,6 +8,7 @@ import de.jeanlucmakiola.clockula.backup.BackupFiles
 import de.jeanlucmakiola.clockula.data.backup.BackupCodec
 import de.jeanlucmakiola.clockula.data.backup.BackupResult
 import kotlinx.coroutines.channels.Channel
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
@@ -37,30 +38,51 @@ class BackupViewModel @Inject internal constructor(
 
     /** `null` means the folder picker was cancelled — a silent no-op. */
     fun onExportFolderPicked(treeUri: String?) {
-        if (treeUri == null) return
+        if (treeUri == null || _busy.value) return
+        _busy.value = true
         viewModelScope.launch {
-            _busy.value = true
-            val json = BackupCodec.encode(engine.export())
-            val event = files.write(treeUri, json).fold(
-                onSuccess = { BackupEvent.ExportSucceeded },
-                onFailure = { BackupEvent.ExportFailed(it.message ?: "unknown error") },
-            )
-            _busy.value = false
-            _events.trySend(event)
+            try {
+                val json = BackupCodec.encode(engine.export())
+                val event = files.write(treeUri, json).fold(
+                    onSuccess = { BackupEvent.ExportSucceeded },
+                    onFailure = {
+                        if (it is CancellationException) throw it
+                        BackupEvent.ExportFailed(it.message ?: "unknown error")
+                    },
+                )
+                _events.trySend(event)
+            } catch (cancelled: CancellationException) {
+                throw cancelled
+            } catch (failure: Exception) {
+                _events.trySend(BackupEvent.ExportFailed(failure.message ?: "unknown error"))
+            } finally {
+                _busy.value = false
+            }
         }
     }
 
     /** `null` means the file picker was cancelled — a silent no-op. Call only after the user has confirmed the replace. */
     fun onImportFilePicked(documentUri: String?) {
-        if (documentUri == null) return
+        if (documentUri == null || _busy.value) return
+        _busy.value = true
         viewModelScope.launch {
-            _busy.value = true
-            val event = files.read(documentUri).fold(
-                onSuccess = { text -> applyImport(text) },
-                onFailure = { BackupEvent.ImportFailed(it.message ?: "unknown error") },
-            )
-            _busy.value = false
-            _events.trySend(event)
+            try {
+                val result = files.read(documentUri)
+                val event = if (result.isSuccess) {
+                    applyImport(result.getOrThrow())
+                } else {
+                    val failure = result.exceptionOrNull()
+                    if (failure is CancellationException) throw failure
+                    BackupEvent.ImportFailed(failure?.message ?: "unknown error")
+                }
+                _events.trySend(event)
+            } catch (cancelled: CancellationException) {
+                throw cancelled
+            } catch (failure: Exception) {
+                _events.trySend(BackupEvent.ImportFailed(failure.message ?: "unknown error"))
+            } finally {
+                _busy.value = false
+            }
         }
     }
 

app/src/test/java/de/jeanlucmakiola/clockula/backup/BackupEngineTest.kt

@@ -159,4 +159,27 @@ class BackupEngineTest {
         assertThat(target.settings.timerPresets.first())
             .containsExactly(1.minutes, 3.minutes, 9.minutes).inOrder()
     }
+
+    @Test
+    fun `failed database replace restores settings and the previous ring`(@TempDir tempDir: Path) = runTest {
+        val harness = backupHarness(tempDir.resolve("failedReplace").also { it.toFile().mkdirs() })
+        harness.givenAlarm(alarmAt(id = 1L, hour = 7, minute = 0, repeatDays = RepeatDays.EVERY_DAY))
+        harness.givenRinging(1L, instantAt(BERLIN, "2026-06-10T07:00"))
+        harness.settings.setThemeMode(ThemeMode.DARK)
+        val replacement = harness.backupEngine.export().copy(
+            alarms = emptyList(),
+            settings = harness.backupEngine.export().settings.copy(themeMode = ThemeMode.LIGHT),
+        )
+        harness.backupDao.failureBeforeReplace = IllegalStateException("database unavailable")
+
+        val failure = runCatching { harness.backupEngine.import(replacement) }.exceptionOrNull()
+
+        assertThat(failure).isInstanceOf(IllegalStateException::class.java)
+        assertThat(harness.storedAlarms()).hasSize(1)
+        assertThat(harness.settings.themeMode.first()).isEqualTo(ThemeMode.DARK)
+        assertThat(harness.alarmEngine.ringSession()?.alarm?.id).isEqualTo(1L)
+        assertThat(harness.ring.events).contains(RingEvent.Stopped(1L))
+        assertThat(harness.ring.events.last()).isEqualTo(RingEvent.Started(1L))
+    }
+
 }

app/src/test/java/de/jeanlucmakiola/clockula/data/backup/BackupCodecTest.kt

@@ -238,6 +238,21 @@ class BackupCodecTest {
         assertThat(BackupCodec.decode("not json at all")).isInstanceOf(BackupResult.Failure::class.java)
     }
 
+    @Test
+    fun `oversized document is rejected before parsing`() {
+        val result = BackupCodec.decode(" ".repeat(8 * 1024 * 1024 + 1))
+        assertThat(result).isInstanceOf(BackupResult.Failure::class.java)
+    }
+
+    @Test
+    fun `oversized alarm label is rejected`() {
+        val json = minimalDocument().replace(
+            "\"alarms\": []",
+            "\"alarms\": [{\"hour\":7,\"minute\":0,\"label\":\"${"x".repeat(64 * 1024 + 1)}\"}]",
+        )
+        assertThat(BackupCodec.decode(json)).isInstanceOf(BackupResult.Failure::class.java)
+    }
+
     private fun minimalDocument(
         extraTopLevel: String = "",
         format: String = "clockula-backup",

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

@@ -6,6 +6,8 @@ import androidx.datastore.preferences.core.Preferences
 import androidx.datastore.preferences.core.stringPreferencesKey
 import app.cash.turbine.test
 import com.google.common.truth.Truth.assertThat
+import de.jeanlucmakiola.clockula.domain.ClockDefaults
+import de.jeanlucmakiola.clockula.domain.backup.BackupSettings
 import de.jeanlucmakiola.floret.prefs.Appearance
 import de.jeanlucmakiola.floret.prefs.PrefStore
 import de.jeanlucmakiola.floret.prefs.ThemeMode
@@ -20,6 +22,7 @@ import kotlinx.coroutines.test.runTest
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.io.TempDir
 import java.nio.file.Path
+import kotlin.time.Duration.Companion.minutes
 
 /**
  * Clockula's appearance preferences over a real DataStore, one temp file per
@@ -64,6 +67,25 @@ class SettingsPrefsTest {
         assertThat(prefs.themeMode.first()).isEqualTo(ThemeMode.DARK)
     }
 
+    @Test
+    fun `backup settings are applied together using normal preference clamps`(@TempDir tempDir: Path) = runTest {
+        val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir)))
+        prefs.applyBackup(
+            BackupSettings(
+                themeMode = ThemeMode.DARK,
+                dynamicColor = false,
+                defaults = ClockDefaults(snoozeMinutes = 99, volumeRampSeconds = 99),
+                timerPresets = listOf(5.minutes, 1.minutes, 5.minutes),
+            ),
+        )
+
+        assertThat(prefs.themeMode.first()).isEqualTo(ThemeMode.DARK)
+        assertThat(prefs.dynamicColor.first()).isFalse()
+        assertThat(prefs.currentDefaults().snoozeMinutes).isEqualTo(60)
+        assertThat(prefs.currentDefaults().volumeRampSeconds).isEqualTo(60)
+        assertThat(prefs.timerPresets.first()).containsExactly(1.minutes, 5.minutes).inOrder()
+    }
+
     @Test
     fun `the dynamic colour choice round-trips`(@TempDir tempDir: Path) = runTest {
         val prefs = SettingsPrefs(PrefStore(newDataStore(tempDir)))

app/src/test/java/de/jeanlucmakiola/clockula/testing/BackupHarness.kt

@@ -102,7 +102,6 @@ class BackupHarness(
     val backupEngine: BackupEngine = BackupEngine(
         repository = backupRepository,
         settings = settings,
-        timers = timers,
         alarmEngine = alarmEngine,
         timerEngine = timerEngine,
         wallClock = wallClock,

app/src/test/java/de/jeanlucmakiola/clockula/testing/FakeDaos.kt

@@ -252,6 +252,17 @@ class FakeBackupDao(
     private val worldClockDao: FakeWorldClockDao,
 ) : BackupDao() {
 
+    var failureBeforeReplace: RuntimeException? = null
+
+    override suspend fun replaceAll(
+        alarms: List<AlarmEntity>,
+        timers: List<TimerEntity>,
+        worldClocks: List<WorldClockEntity>,
+    ) {
+        failureBeforeReplace?.let { throw it }
+        super.replaceAll(alarms, timers, worldClocks)
+    }
+
     override suspend fun deleteAllAlarms() {
         for (row in alarmDao.stored) alarmDao.deleteById(row.id)
     }

app/src/test/java/de/jeanlucmakiola/clockula/ui/settings/BackupViewModelTest.kt

@@ -0,0 +1,55 @@
+package de.jeanlucmakiola.clockula.ui.settings
+
+import com.google.common.truth.Truth.assertThat
+import de.jeanlucmakiola.clockula.backup.BackupFiles
+import de.jeanlucmakiola.clockula.data.backup.BackupCodec
+import de.jeanlucmakiola.clockula.testing.MainDispatcherExtension
+import de.jeanlucmakiola.clockula.testing.backupHarness
+import kotlinx.coroutines.flow.first
+import kotlinx.coroutines.test.runTest
+import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.extension.RegisterExtension
+import org.junit.jupiter.api.io.TempDir
+import java.nio.file.Path
+
+class BackupViewModelTest {
+
+    @JvmField
+    @RegisterExtension
+    val main = MainDispatcherExtension()
+
+    @Test
+    fun `a thrown export failure reports an error and clears busy`(@TempDir tempDir: Path) = runTest(main.dispatcher) {
+        val harness = backupHarness(tempDir)
+        val files = object : BackupFiles {
+            override suspend fun write(treeUri: String, content: String): Result<Unit> =
+                throw IllegalStateException("provider unavailable")
+
+            override suspend fun read(documentUri: String): Result<String> = error("unused")
+        }
+        val viewModel = BackupViewModel(harness.backupEngine, files)
+
+        viewModel.onExportFolderPicked("content://folder")
+
+        assertThat(viewModel.events.first()).isEqualTo(BackupEvent.ExportFailed("provider unavailable"))
+        assertThat(viewModel.busy.value).isFalse()
+    }
+
+    @Test
+    fun `a failed restore reports an error and clears busy`(@TempDir tempDir: Path) = runTest(main.dispatcher) {
+        val harness = backupHarness(tempDir)
+        val json = BackupCodec.encode(harness.backupEngine.export())
+        harness.backupDao.failureBeforeReplace = IllegalStateException("database unavailable")
+        val files = object : BackupFiles {
+            override suspend fun write(treeUri: String, content: String): Result<Unit> = error("unused")
+
+            override suspend fun read(documentUri: String): Result<String> = Result.success(json)
+        }
+        val viewModel = BackupViewModel(harness.backupEngine, files)
+
+        viewModel.onImportFilePicked("content://backup")
+
+        assertThat(viewModel.events.first()).isEqualTo(BackupEvent.ImportFailed("database unavailable"))
+        assertThat(viewModel.busy.value).isFalse()
+    }
+}

docs/BACKUP.md

@@ -42,15 +42,26 @@ written here.
   challenge` or `theme_mode` from a future version, say) degrades to that
   field's default, the same forgiving-read posture `AlarmMapper` already uses
   for a corrupt Room row. It does not fail the import.
+- Import and export are limited to 8 MiB. Import also rejects more than 10,000
+  alarms, 10,000 timers, 1,000 world clocks, oversized labels or ringtone URIs,
+  and more than 1,000 timer presets. These limits keep a picked document from
+  exhausting memory or creating an impractical number of rows.
 
 ## Import semantics
 
 Import **replaces all state**: alarms, timers and world clocks are deleted
 and re-created from the file, and every setting the file carries is written
 over the current value. This is not a merge, and the backup screen says so
-before the user confirms. A ringing alarm is dismissed and any running timer
-is deleted *before* the replace, so the import never leaves an orphaned ring
-session; alarm/timer/stopwatch scheduling is re-resolved immediately after.
+before the user confirms. Settings are written in one DataStore transaction;
+the three Room tables are replaced in one database transaction. The live ring
+and timer services are paused while the tables change. If the database replace
+fails, the old rows remain, the previous settings are restored, and the ring
+and timer registrations are resumed. Scheduling is reconciled after either
+outcome. An imported timer always starts idle.
+
+Export writes a complete temporary document before removing an older backup.
+If the provider refuses the final rename, the complete temporary document is
+kept in the chosen folder and the app reports the failure with its filename.
 
 Every imported alarm and timer gets a **fresh id and fresh timestamps**.
 The backup carries no id and no `created_at`/`updated_at`, since those are
@@ -127,8 +138,8 @@ Field names mirror `ClockPrefs`'/`SettingsPrefs`' own on-disk keys. Older
 files may also carry `default_timer_duration_millis`, from a default timer
 length setting that has since been removed; it is ignored on import like any
 other unknown field. Every
-numeric/duration value is clamped on *apply*, through the existing pref
-setters, never on parse.
+numeric/duration value is clamped on *apply* using the same ranges and encoding
+as the existing preference setters, never on parse.
 
 ## Ringtone URIs