From 52dceae595863d5e8641adc2888c64d39c012cb9 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:06:47 -0400 Subject: [PATCH 01/16] Library picks back up when the server returns The connected rising edge only flushed queued progress, so after an outage the library and its "Last sync failed" banner stayed stale until a manual Retry or the next 5 minute tick. A returning server now resyncs whenever the last recorded attempt failed or was partial. SYNCING counts as live so a sync's own status flip never retriggers it. A background sync that brings data back now re-queries the shelf instead of only clearing the banner. Library loads probe /ping before the remote fetch. A live Tailscale interface kept isOnline true with the server gone, so an offline cold start sat on a spinner for the 30s connect timeout before showing saved books. When the connection drops and the auto downloaded-only filter leaves nothing on screen, the empty state now says "Nothing Downloaded" instead of blaming filters, and the banner stops claiming saved books are showing. Co-Authored-By: Claude Opus 5.5 --- .../ninelivesaudio/app/service/SyncManager.kt | 41 ++++++++++++ .../app/ui/library/LibraryScreen.kt | 34 ++++++++-- .../app/ui/library/LibraryViewModel.kt | 24 ++++++- .../app/service/ServerReturnResyncTest.kt | 63 +++++++++++++++++++ .../ui/library/LibraryOfflineEmptyCopyTest.kt | 63 +++++++++++++++++++ 5 files changed, 218 insertions(+), 7 deletions(-) create mode 100644 app/src/test/java/com/ninelivesaudio/app/service/ServerReturnResyncTest.kt create mode 100644 app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryOfflineEmptyCopyTest.kt diff --git a/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt b/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt index 7bfa471b..8fd114ca 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt @@ -127,6 +127,27 @@ class SyncManager @Inject constructor( } } + // Resync when the server comes back after a failed or partial sync. + // The flush above only pushes queued progress, so without this the + // library and its "Last sync failed" banner stayed stale until a + // manual Retry or the next periodic tick (up to 5 minutes). + // SYNCING counts as live so a sync's own status flip is not an edge. + launch { + combine( + connectivityMonitor.connectionStatus, + settingsManager.settings, + ) { status, settings -> isServerSessionLive(status, settings.appMode) } + .distinctUntilChanged() + .collect { live -> + if (live && shouldResyncOnServerReturn( + settingsManager.currentSettings.lastSyncForCurrentServer()?.result, + ) + ) { + syncNow() + } + } + } + launch { var previousMode: AppMode? = null settingsManager.settings @@ -524,6 +545,26 @@ internal suspend fun performModeSwitchReconnect( syncNow() } +/** + * True while the server is usable for this session. SYNCING is included so + * the brief SYNCING status a sync sets on itself never reads as the server + * leaving and coming back, which would retrigger the resync in a loop. + */ +internal fun isServerSessionLive( + status: ConnectivityMonitor.ConnectionStatus, + appMode: AppMode, +): Boolean = appMode != AppMode.LOCAL && + (status == ConnectivityMonitor.ConnectionStatus.CONNECTED || + status == ConnectivityMonitor.ConnectionStatus.SYNCING) + +/** + * A returning server only needs a resync when the last recorded attempt did + * not fully succeed. No record means the startup sync has not run yet and + * will cover it, so the edge stays quiet rather than doubling that sync. + */ +internal fun shouldResyncOnServerReturn(lastResult: SyncResult?): Boolean = + lastResult != null && lastResult != SyncResult.SUCCESS + internal fun shouldReconnectForModeTransition( previousMode: AppMode, newMode: AppMode, diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryScreen.kt b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryScreen.kt index 01df1c1d..7997beb5 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryScreen.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryScreen.kt @@ -223,6 +223,7 @@ fun LibraryScreen( SyncWarningBanner( result = warning, onRetry = viewModel::refresh, + showingSavedBooks = false, modifier = Modifier.padding(horizontal = 18.dp), ) } @@ -1053,12 +1054,9 @@ private fun SyncWarningBanner( result: SyncResult, onRetry: () -> Unit, modifier: Modifier = Modifier, + showingSavedBooks: Boolean = true, ) { - val message = when (result) { - SyncResult.PARTIAL -> "Last sync was incomplete. Showing saved books." - SyncResult.FAILED -> "Last sync failed. Showing saved books." - SyncResult.SUCCESS -> return - } + val message = syncWarningMessage(result, showingSavedBooks) ?: return Surface( modifier = modifier.fillMaxWidth(), color = NineLivesTheme.colors.archiveVoidSurface, @@ -1099,6 +1097,8 @@ private fun EmptyState(uiState: LibraryViewModel.UiState) { val (title, subtitle) = when { uiState.searchQuery.isNotBlank() -> CopyStyleGuide.Search.NO_RESULTS to (searchFlavor ?: "Nothing in the Archive matches \"${uiState.searchQuery}\".") + isOfflineDownloadedOnlyEmpty(uiState) -> + "Nothing Downloaded" to "Streamed books come back when the server does." uiState.connectionStatus == com.ninelivesaudio.app.service.ConnectivityMonitor.ConnectionStatus.OFFLINE -> "The Archive Is Sealed" to (CopyEngine.getEmptyStateFlavor( "Connection lost. Check your network.", @@ -1138,3 +1138,27 @@ private fun EmptyState(uiState: LibraryViewModel.UiState) { ) } } + +/** + * Banner copy for a degraded sync. The "showing saved books" half is only + * true when the shelf below actually has books on it. + */ +internal fun syncWarningMessage(result: SyncResult, showingSavedBooks: Boolean): String? { + val lead = when (result) { + SyncResult.PARTIAL -> "Last sync was incomplete." + SyncResult.FAILED -> "Last sync failed." + SyncResult.SUCCESS -> return null + } + return if (showingSavedBooks) "$lead Showing saved books." else lead +} + +/** + * The server dropped, the shelf auto-switched to downloaded books, and there + * are none. Saved streamed books exist but cannot play, so say that instead + * of blaming a filter the user never touched. + */ +internal fun isOfflineDownloadedOnlyEmpty(uiState: LibraryViewModel.UiState): Boolean = + uiState.showDownloadedOnly && + uiState.totalBookCount > 0 && + uiState.connectionStatus != com.ninelivesaudio.app.service.ConnectivityMonitor.ConnectionStatus.CONNECTED && + uiState.connectionStatus != com.ninelivesaudio.app.service.ConnectivityMonitor.ConnectionStatus.SYNCING diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt index ae863de5..2c61e42f 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt @@ -14,6 +14,7 @@ import com.ninelivesaudio.app.data.repository.ReconciledServerLibraryList import com.ninelivesaudio.app.domain.model.AppMode import com.ninelivesaudio.app.domain.model.AppSettings import com.ninelivesaudio.app.domain.model.AudioBook +import com.ninelivesaudio.app.domain.model.LastSyncRecord import com.ninelivesaudio.app.domain.model.Library import com.ninelivesaudio.app.domain.model.SyncResult import com.ninelivesaudio.app.service.ConnectivityMonitor @@ -85,6 +86,19 @@ internal data class DownloadedOnlyFilterState( val autoDownloadedOnly: Boolean, ) +/** + * A new sync outcome that brought data back (not a plain failure) should + * re-query the shelf. The first record seen is skipped because the initial + * load already queried it. + */ +internal fun shouldRequeryShelfAfterSync( + previousSequence: Long?, + record: LastSyncRecord?, +): Boolean = previousSequence != null && + record != null && + record.outcomeSequence != previousSequence && + record.result != SyncResult.FAILED + internal fun decideDownloadedOnlyFilter( previousStatus: ConnectionStatus, newStatus: ConnectionStatus, @@ -247,6 +261,7 @@ class LibraryViewModel @Inject constructor( } .distinctUntilChanged() .collect { record -> + val previousSequence = _uiState.value.lastSyncSequence _uiState.update { it.copy( lastSyncResult = record?.result, @@ -254,6 +269,9 @@ class LibraryViewModel @Inject constructor( lastSyncFailedLibraryIds = record?.failedLibraryIds, ) } + // A background sync (reconnect or periodic) wrote new rows. + // Re-query so the shelf shows them, not just a cleared banner. + if (shouldRequeryShelfAfterSync(previousSequence, record)) applyFilter() } } @@ -279,7 +297,7 @@ class LibraryViewModel @Inject constructor( if (shouldSyncOnLibraryLoad( isLocalLibrary = false, isOnline = connectivityMonitor.isOnline.value, - ) + ) && connectivityMonitor.checkServerReachable() ) { refreshRemoteLibraryList( readCached = { @@ -353,10 +371,12 @@ class LibraryViewModel @Inject constructor( // connectivity. In airplane mode the old code attempted syncLibraryItems // regardless, leaving the switch spinning on a doomed request until the // OkHttp timeout. Skipping the sync lets cached data load instantly. + // A live VPN (Tailscale) keeps isOnline true with the server gone, + // so the short /ping probe decides before the 30s request does. if (shouldSyncOnLibraryLoad( isLocalLibrary = selected?.isLocal == true, isOnline = connectivityMonitor.isOnline.value, - ) + ) && connectivityMonitor.checkServerReachable() ) { itemResult = refreshSelectedLibraryItems( libraryId = libraryId, diff --git a/app/src/test/java/com/ninelivesaudio/app/service/ServerReturnResyncTest.kt b/app/src/test/java/com/ninelivesaudio/app/service/ServerReturnResyncTest.kt new file mode 100644 index 00000000..eaf16095 --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/service/ServerReturnResyncTest.kt @@ -0,0 +1,63 @@ +package com.ninelivesaudio.app.service + +import com.ninelivesaudio.app.domain.model.AppMode +import com.ninelivesaudio.app.domain.model.SyncResult +import com.ninelivesaudio.app.service.ConnectivityMonitor.ConnectionStatus +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class ServerReturnResyncTest { + + @Test + fun `a returning server resyncs after a failed or partial sync`() { + assertTrue(shouldResyncOnServerReturn(SyncResult.FAILED)) + assertTrue(shouldResyncOnServerReturn(SyncResult.PARTIAL)) + } + + @Test + fun `a returning server leaves a clean or missing record alone`() { + assertFalse(shouldResyncOnServerReturn(SyncResult.SUCCESS)) + // No record yet: the startup sync owns the first attempt. + assertFalse(shouldResyncOnServerReturn(null)) + } + + @Test + fun `server session is live only when connected or syncing outside local mode`() { + assertTrue(isServerSessionLive(ConnectionStatus.CONNECTED, AppMode.AUDIOBOOKSHELF)) + assertTrue(isServerSessionLive(ConnectionStatus.SYNCING, AppMode.AUDIOBOOKSHELF)) + assertFalse(isServerSessionLive(ConnectionStatus.SERVER_UNREACHABLE, AppMode.AUDIOBOOKSHELF)) + assertFalse(isServerSessionLive(ConnectionStatus.OFFLINE, AppMode.AUDIOBOOKSHELF)) + assertFalse(isServerSessionLive(ConnectionStatus.CONNECTED, AppMode.LOCAL)) + } + + @Test + fun `a sync's own status flip is not a server return`() { + // CONNECTED -> SYNCING -> CONNECTED is what every sync does to the + // status. If that produced a rising edge, a sync that ended PARTIAL + // would retrigger itself forever. + val edges = listOf( + ConnectionStatus.CONNECTED, + ConnectionStatus.SYNCING, + ConnectionStatus.CONNECTED, + ).map { isServerSessionLive(it, AppMode.AUDIOBOOKSHELF) }.distinctUntilChangedList() + assertEquals(listOf(true), edges) + } + + @Test + fun `an outage and recovery produces exactly one rising edge`() { + val edges = listOf( + ConnectionStatus.CONNECTED, + ConnectionStatus.SERVER_UNREACHABLE, + ConnectionStatus.OFFLINE, + ConnectionStatus.CONNECTED, + ConnectionStatus.SYNCING, + ConnectionStatus.CONNECTED, + ).map { isServerSessionLive(it, AppMode.AUDIOBOOKSHELF) }.distinctUntilChangedList() + assertEquals(listOf(true, false, true), edges) + } + + private fun List.distinctUntilChangedList(): List = + fold(mutableListOf()) { acc, v -> if (acc.lastOrNull() != v) acc.add(v); acc } +} diff --git a/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryOfflineEmptyCopyTest.kt b/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryOfflineEmptyCopyTest.kt new file mode 100644 index 00000000..8c1602f7 --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryOfflineEmptyCopyTest.kt @@ -0,0 +1,63 @@ +package com.ninelivesaudio.app.ui.library + +import com.ninelivesaudio.app.domain.model.LastSyncRecord +import com.ninelivesaudio.app.domain.model.SyncResult +import com.ninelivesaudio.app.service.ConnectivityMonitor.ConnectionStatus +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +class LibraryOfflineEmptyCopyTest { + + private fun state( + downloadedOnly: Boolean = true, + total: Int = 9, + status: ConnectionStatus = ConnectionStatus.SERVER_UNREACHABLE, + ) = LibraryViewModel.UiState( + showDownloadedOnly = downloadedOnly, + totalBookCount = total, + connectionStatus = status, + ) + + @Test + fun `offline downloaded-only with saved books says nothing is downloaded`() { + assertTrue(isOfflineDownloadedOnlyEmpty(state())) + assertTrue(isOfflineDownloadedOnlyEmpty(state(status = ConnectionStatus.OFFLINE))) + } + + @Test + fun `connected, unfiltered, or truly empty shelves keep their old copy`() { + assertFalse(isOfflineDownloadedOnlyEmpty(state(status = ConnectionStatus.CONNECTED))) + assertFalse(isOfflineDownloadedOnlyEmpty(state(status = ConnectionStatus.SYNCING))) + assertFalse(isOfflineDownloadedOnlyEmpty(state(downloadedOnly = false))) + assertFalse(isOfflineDownloadedOnlyEmpty(state(total = 0))) + } + + @Test + fun `banner only claims saved books when the shelf shows some`() { + assertEquals("Last sync failed. Showing saved books.", syncWarningMessage(SyncResult.FAILED, true)) + assertEquals("Last sync failed.", syncWarningMessage(SyncResult.FAILED, false)) + assertEquals("Last sync was incomplete.", syncWarningMessage(SyncResult.PARTIAL, false)) + assertNull(syncWarningMessage(SyncResult.SUCCESS, true)) + } + + private fun record(seq: Long, result: SyncResult) = LastSyncRecord( + result = result, libraryCount = 1, bookCount = 9, completedAtMs = 0L, outcomeSequence = seq, + ) + + @Test + fun `a new successful or partial background sync re-queries the shelf`() { + assertTrue(shouldRequeryShelfAfterSync(4L, record(5L, SyncResult.SUCCESS))) + assertTrue(shouldRequeryShelfAfterSync(4L, record(5L, SyncResult.PARTIAL))) + } + + @Test + fun `failed, repeated, or first-seen records do not re-query`() { + assertFalse(shouldRequeryShelfAfterSync(4L, record(5L, SyncResult.FAILED))) + assertFalse(shouldRequeryShelfAfterSync(5L, record(5L, SyncResult.SUCCESS))) + assertFalse(shouldRequeryShelfAfterSync(null, record(5L, SyncResult.SUCCESS))) + assertFalse(shouldRequeryShelfAfterSync(4L, null)) + } +} From 6bd77e5c9105aa1bfbf62acd34bbc7f077f5df5b Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:13:23 -0400 Subject: [PATCH 02/16] Pause, cancel and delete stick while a book is downloading The download engine rewrites its own row as Downloading about ten times a second. Pause wrote Paused and only then stopped the worker, and cancel deleted the row first, so one late progress tick undid either of them: a cancelled download came back, a paused one kept going. Delete could have the finishing engine mark the book downloaded again after its files were gone. All three now do what entitlement changes already did. When the book is actively downloading, stop the worker, wait until it has really stopped, then write, then restart the queue for anything else. Pause re-reads the row after the stop so it keeps the latest progress, and leaves a book that finished in that window alone. Co-Authored-By: Claude Opus 5.5 --- .../app/service/DownloadManager.kt | 49 +++++++++++++------ .../app/service/download/DownloadWork.kt | 18 +++++++ .../download/UserDownloadWriteOrderTest.kt | 36 ++++++++++++++ 3 files changed, 89 insertions(+), 14 deletions(-) create mode 100644 app/src/test/java/com/ninelivesaudio/app/service/download/UserDownloadWriteOrderTest.kt diff --git a/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt b/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt index b7bf2984..2928739f 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt @@ -20,6 +20,7 @@ import com.ninelivesaudio.app.service.download.DownloadQueueWorker import com.ninelivesaudio.app.service.download.estimateTotalBytes import com.ninelivesaudio.app.service.download.DownloadSlotStore import com.ninelivesaudio.app.service.download.selectNextDownload +import com.ninelivesaudio.app.service.download.writeAfterEngineStops import dagger.hilt.android.qualifiers.ApplicationContext import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableSharedFlow @@ -386,8 +387,7 @@ class DownloadManager @Inject constructor( } if (!hasLosingDownload) return - workManager.cancelUniqueWork(DOWNLOAD_WORK_NAME) - awaitDrainStopped() + stopDrainAndAwait() slotMutex.withLock { downloadItemDao.getAll() @@ -412,6 +412,11 @@ class DownloadManager @Inject constructor( * gives up and proceeds, which is the same risk profile as before this * function existed. */ + private suspend fun stopDrainAndAwait() { + workManager.cancelUniqueWork(DOWNLOAD_WORK_NAME) + awaitDrainStopped() + } + private suspend fun awaitDrainStopped() { val deadline = System.currentTimeMillis() + WORKER_STOP_TIMEOUT_MS while (System.currentTimeMillis() < deadline) { @@ -453,9 +458,15 @@ class DownloadManager @Inject constructor( suspend fun pauseDownload(downloadId: String) { val entity = downloadItemDao.getById(downloadId) ?: return val wasDownloading = entity.status == DownloadStatus.Downloading.ordinal - downloadItemDao.upsert(entity.copy(status = DownloadStatus.Paused.ordinal)) + writeAfterEngineStops(wasDownloading, { stopDrainAndAwait() }) { + // Re-read after the stop: the engine's last tick moved the bytes on, + // and it may have finished the book in the meantime. + val latest = downloadItemDao.getById(downloadId) ?: return@writeAfterEngineStops + if (latest.status == DownloadStatus.Completed.ordinal) return@writeAfterEngineStops + downloadItemDao.upsert(latest.copy(status = DownloadStatus.Paused.ordinal)) + } if (wasDownloading) { - // Stop the engine on this book and let the drain continue with the rest. + // The engine was stopped above; let the drain continue with the rest. enqueueDrain(replace = true) } } @@ -492,7 +503,9 @@ class DownloadManager @Inject constructor( suspend fun cancelDownload(downloadId: String) { val entity = downloadItemDao.getById(downloadId) val wasDownloading = entity?.status == DownloadStatus.Downloading.ordinal - downloadItemDao.deleteById(downloadId) + writeAfterEngineStops(wasDownloading, { stopDrainAndAwait() }) { + downloadItemDao.deleteById(downloadId) + } if (downloadItemDao.getDownloadable().isEmpty()) { // Nothing left to download: stop the drain and clear the notification @@ -500,18 +513,30 @@ class DownloadManager @Inject constructor( workManager.cancelUniqueWork(DOWNLOAD_WORK_NAME) DownloadNotifications.clearAll(context) } else if (wasDownloading) { - // More queued: stop the engine on the cancelled book and continue. + // More queued: the engine was stopped above, so continue with the rest. enqueueDrain(replace = true) } } /** Delete a download's files and DB record. */ suspend fun deleteDownload(audioBookId: String) { + val downloadEntity = downloadItemDao.getByAudioBookId(audioBookId) + val wasDownloading = downloadEntity?.status == DownloadStatus.Downloading.ordinal + writeAfterEngineStops(wasDownloading, { stopDrainAndAwait() }) { + deleteDownloadedFilesAndRows(audioBookId, downloadEntity?.id) + } + if (wasDownloading) { + // Restart the drain for whatever else is queued. + enqueueDrain(replace = true) + } + } + + private suspend fun deleteDownloadedFilesAndRows(audioBookId: String, downloadId: String?) { + // Read the book only now, after any engine on it has stopped, so a + // completion that landed during the stop is the state being undone. // Use the actual localPath stored on the audiobook — this matches the path // set by the engine (basePath/Author - Title), not basePath/audioBookId. val bookEntity = audioBookDao.getById(audioBookId) - val downloadEntity = downloadItemDao.getByAudioBookId(audioBookId) - val wasDownloading = downloadEntity?.status == DownloadStatus.Downloading.ordinal withContext(Dispatchers.IO) { val localPath = bookEntity?.localPath @@ -525,12 +550,8 @@ class DownloadManager @Inject constructor( // above, so drop its reference too. audioBookDao.upsert(bookEntity.copy(isDownloaded = 0, localPath = null, localCoverPath = null)) } - if (downloadEntity != null) { - downloadItemDao.deleteById(downloadEntity.id) - } - if (wasDownloading) { - // Stop the engine if it was mid-download on this book. - enqueueDrain(replace = true) + if (downloadId != null) { + downloadItemDao.deleteById(downloadId) } } diff --git a/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt b/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt index fce5387a..8793fb24 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt @@ -26,3 +26,21 @@ fun selectNextDownload(items: List): DownloadItem? = compareByDescending { it.status == DownloadStatus.Downloading } .thenBy { it.startedAt ?: Long.MAX_VALUE } ) + +/** + * Run a user's pause, cancel or delete write so the engine cannot undo it. + * + * DownloadEngine upserts a stale Downloading snapshot on every progress tick + * and marks the book downloaded when it finishes. A write that lands while it + * is still streaming this book gets clobbered straight back. So when the + * engine is on this book, [stopEngine] (cancel plus confirmed termination) + * runs first, and only then [write]. + */ +internal suspend fun writeAfterEngineStops( + engineActive: Boolean, + stopEngine: suspend () -> Unit, + write: suspend () -> T, +): T { + if (engineActive) stopEngine() + return write() +} diff --git a/app/src/test/java/com/ninelivesaudio/app/service/download/UserDownloadWriteOrderTest.kt b/app/src/test/java/com/ninelivesaudio/app/service/download/UserDownloadWriteOrderTest.kt new file mode 100644 index 00000000..21579d42 --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/service/download/UserDownloadWriteOrderTest.kt @@ -0,0 +1,36 @@ +package com.ninelivesaudio.app.service.download + +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * Pause, cancel and delete on the book the engine is streaming must stop the + * engine BEFORE writing. DownloadEngine upserts a stale Downloading snapshot + * about ten times a second, so a write that lands first gets clobbered by the + * next progress tick (a cancelled row comes back, a paused one keeps going). + */ +class UserDownloadWriteOrderTest { + + @Test + fun `an active download is stopped before the user's write lands`() = runBlocking { + val events = mutableListOf() + writeAfterEngineStops( + engineActive = true, + stopEngine = { events += "stop" }, + write = { events += "write" }, + ) + assertEquals(listOf("stop", "write"), events) + } + + @Test + fun `an idle row is written without stopping the drain`() = runBlocking { + val events = mutableListOf() + writeAfterEngineStops( + engineActive = false, + stopEngine = { events += "stop" }, + write = { events += "write" }, + ) + assertEquals(listOf("write"), events) + } +} From 5ad6b3dd86664c314d7039e8fd5b80c80c1bf919 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:16:47 -0400 Subject: [PATCH 03/16] The self-signed certificate toggle works without a restart The HTTP client is built before settings load, so it always saw the toggle as off and never installed the self-signed handling. Turning it on only saved the preference. Servers with a self-signed certificate could not connect no matter what the switch said. The trust check and hostname check are now always on the client and read the toggle on every connection. Off, they hand the whole decision to the platform trust store and OkHttp's default hostname check, the same check the client made before. On, the existing trust-on-first-use behavior and host match apply, unchanged. Co-Authored-By: Claude Opus 5.5 --- .../data/remote/SelfSignedCertTrustManager.kt | 277 +++++++++++------- .../data/remote/SelfSignedCertTrustTest.kt | 188 ++++++++++++ 2 files changed, 367 insertions(+), 98 deletions(-) create mode 100644 app/src/test/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustTest.kt diff --git a/app/src/main/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustManager.kt b/app/src/main/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustManager.kt index 673233b9..4904cfec 100644 --- a/app/src/main/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustManager.kt @@ -3,13 +3,21 @@ package com.ninelivesaudio.app.data.remote import android.util.Log import com.ninelivesaudio.app.service.SettingsManager import okhttp3.OkHttpClient +import okhttp3.internal.tls.OkHostnameVerifier +import java.net.Socket import java.net.URI +import java.security.KeyStore import java.security.MessageDigest import java.security.SecureRandom import java.security.cert.CertificateException import java.security.cert.X509Certificate +import javax.net.ssl.HostnameVerifier import javax.net.ssl.SSLContext +import javax.net.ssl.SSLEngine +import javax.net.ssl.SSLSession import javax.net.ssl.TrustManager +import javax.net.ssl.TrustManagerFactory +import javax.net.ssl.X509ExtendedTrustManager import javax.net.ssl.X509TrustManager /** @@ -24,7 +32,8 @@ import javax.net.ssl.X509TrustManager * ## Safeguards * * 1. **Opt-in only:** `allowSelfSignedCertificates` defaults to `false` in [AppSettings]. - * The permissive trust manager is never installed unless the user explicitly enables it. + * The gate is always installed but reads the pref per handshake. While it is off, + * every check goes to the platform trust manager and OkHttp's default verifier. * 2. **Host-scoped:** The custom [javax.net.ssl.HostnameVerifier] restricts certificate * acceptance to the single hostname extracted from the configured server URL. Requests * to any other host use standard certificate validation. @@ -37,8 +46,6 @@ import javax.net.ssl.X509TrustManager * configured host on first successful handshake and rejects future mismatches. */ object SelfSignedCertTrustManager { - private const val TAG = "SelfSignedTrustManager" - class CertificateFingerprintMismatchException( val host: String, val expectedFingerprint: String, @@ -50,125 +57,199 @@ object SelfSignedCertTrustManager { /** - * Configures the OkHttpClient.Builder to accept self-signed certificates - * ONLY for the server host from settings, and ONLY if the user opted in. + * Installs the pref-gated trust manager and hostname verifier on the client. * - * When `allowSelfSignedCertificates` is `false` (the default), this method is a - * no-op — the default OkHttp trust manager validates certificate chains against - * the system CA store as normal. + * Always installed, because Hilt builds this client before settings load, so + * a check made here would always see the default (off) and flipping the + * toggle would never take effect. Both read `allowSelfSignedCertificates` + * on every handshake instead. Off, they hand straight to the platform trust + * manager and OkHttp's default hostname verifier, so the default path is the + * same check OkHttp would do on its own. On, trust-on-first-use applies. */ fun OkHttpClient.Builder.configureSelfSignedCerts( settingsManager: SettingsManager, ): OkHttpClient.Builder { - // Only install the permissive trust manager when the user has opted in. - // Without this guard, the no-op checkServerTrusted() accepts every cert. - val settings = settingsManager.currentSettings - if (!settings.allowSelfSignedCertificates) { - return this - } + val allowSelfSigned = { settingsManager.currentSettings.allowSelfSignedCertificates } + val tofu = TofuServerTrust( + serverUrl = { settingsManager.currentSettings.serverUrl }, + trustedFingerprint = settingsManager::getTrustedCertificateFingerprint, + saveFingerprint = settingsManager::saveTrustedCertificateFingerprint, + ) + + val trustManager = PrefGatedTrustManager(platformTrustManager(), allowSelfSigned, tofu) + val sslContext = SSLContext.getInstance("TLS") + sslContext.init(null, arrayOf(trustManager), SecureRandom()) - val trustManager = object : X509TrustManager { - override fun checkClientTrusted(chain: Array?, authType: String?) { - throw CertificateException("Client certificates not supported") - } + sslSocketFactory(sslContext.socketFactory, trustManager) + hostnameVerifier(PrefGatedHostnameVerifier(OkHostnameVerifier, allowSelfSigned, tofu)) - override fun checkServerTrusted(chain: Array?, authType: String?) { - if (chain.isNullOrEmpty()) { - throw CertificateException("Server certificate chain is empty") - } + return this + } - chain.forEach { cert -> cert.checkValidity() } + /** The platform's default trust manager, the same one OkHttp uses by default. */ + internal fun platformTrustManager(): X509TrustManager { + val factory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()) + factory.init(null as KeyStore?) + return factory.trustManagers.filterIsInstance().first() + } +} - val serverUrl = settingsManager.currentSettings.serverUrl - val configuredHost = try { - URI(serverUrl).host?.lowercase() - } catch (_: Exception) { - null - } +/** + * Off: the platform trust manager, every overload forwarded as is. The socket + * and engine forms matter on Android, where the Network Security Config trust + * manager needs the peer host they carry. On: trust-on-first-use. + */ +internal class PrefGatedTrustManager( + private val platform: X509TrustManager, + private val allowSelfSigned: () -> Boolean, + private val tofu: X509TrustManager, +) : X509ExtendedTrustManager() { + + override fun checkClientTrusted(chain: Array?, authType: String?) { + if (allowSelfSigned()) tofu.checkClientTrusted(chain, authType) + else platform.checkClientTrusted(chain, authType) + } - if (configuredHost.isNullOrEmpty()) { - throw CertificateException("Configured server host is invalid") - } + override fun checkClientTrusted(chain: Array?, authType: String?, socket: Socket?) { + if (allowSelfSigned()) tofu.checkClientTrusted(chain, authType) + else if (platform is X509ExtendedTrustManager) platform.checkClientTrusted(chain, authType, socket) + else platform.checkClientTrusted(chain, authType) + } - val leafCert = chain.first() - val fingerprint = leafCert.sha256Fingerprint() - val trustedFingerprint = settingsManager.getTrustedCertificateFingerprint(configuredHost) - - if (trustedFingerprint == null) { - // First-time connection: accept here. Enrollment happens in the - // hostnameVerifier from this session's own peer certificate, so it - // is bound to the verified hostname and the actual handshake — no - // shared cross-connection state to race or poison. - Log.i(TAG, "TOFU first contact for host=$configuredHost (enrollment pending hostname verification)") - return - } + override fun checkClientTrusted(chain: Array?, authType: String?, engine: SSLEngine?) { + if (allowSelfSigned()) tofu.checkClientTrusted(chain, authType) + else if (platform is X509ExtendedTrustManager) platform.checkClientTrusted(chain, authType, engine) + else platform.checkClientTrusted(chain, authType) + } - if (!fingerprint.equals(trustedFingerprint, ignoreCase = true)) { - Log.e( - TAG, - "TLS fingerprint mismatch for host=$configuredHost" - ) - throw CertificateFingerprintMismatchException( - host = configuredHost, - expectedFingerprint = trustedFingerprint, - actualFingerprint = fingerprint, - ) - } - } + override fun checkServerTrusted(chain: Array?, authType: String?) { + if (allowSelfSigned()) tofu.checkServerTrusted(chain, authType) + else platform.checkServerTrusted(chain, authType) + } + + override fun checkServerTrusted(chain: Array?, authType: String?, socket: Socket?) { + if (allowSelfSigned()) tofu.checkServerTrusted(chain, authType) + else if (platform is X509ExtendedTrustManager) platform.checkServerTrusted(chain, authType, socket) + else platform.checkServerTrusted(chain, authType) + } - override fun getAcceptedIssuers(): Array = emptyArray() + override fun checkServerTrusted(chain: Array?, authType: String?, engine: SSLEngine?) { + if (allowSelfSigned()) tofu.checkServerTrusted(chain, authType) + else if (platform is X509ExtendedTrustManager) platform.checkServerTrusted(chain, authType, engine) + else platform.checkServerTrusted(chain, authType) + } + + override fun getAcceptedIssuers(): Array = + if (allowSelfSigned()) tofu.acceptedIssuers else platform.acceptedIssuers +} + +/** Off: the platform verifier's answer. On: the configured host only, plus TOFU enrollment. */ +internal class PrefGatedHostnameVerifier( + private val platform: HostnameVerifier, + private val allowSelfSigned: () -> Boolean, + private val tofu: HostnameVerifier, +) : HostnameVerifier { + override fun verify(hostname: String, session: SSLSession): Boolean = + if (allowSelfSigned()) tofu.verify(hostname, session) else platform.verify(hostname, session) +} + +/** + * Trust-on-first-use for the configured server, used only while the user has + * opted in. Unchanged from when it was built inline in configureSelfSignedCerts. + */ +internal class TofuServerTrust( + private val serverUrl: () -> String, + private val trustedFingerprint: (String) -> String?, + private val saveFingerprint: (String, String) -> Unit, + private val log: (Int, String) -> Unit = { priority, message -> Log.println(priority, TAG, message) }, +) : X509TrustManager, HostnameVerifier { + + override fun checkClientTrusted(chain: Array?, authType: String?) { + throw CertificateException("Client certificates not supported") + } + + override fun checkServerTrusted(chain: Array?, authType: String?) { + if (chain.isNullOrEmpty()) { + throw CertificateException("Server certificate chain is empty") } - val sslContext = SSLContext.getInstance("TLS") - sslContext.init(null, arrayOf(trustManager), SecureRandom()) + chain.forEach { cert -> cert.checkValidity() } - sslSocketFactory(sslContext.socketFactory, trustManager) + val configuredHost = try { + URI(serverUrl()).host?.lowercase() + } catch (_: Exception) { + null + } + + if (configuredHost.isNullOrEmpty()) { + throw CertificateException("Configured server host is invalid") + } - hostnameVerifier { hostname, session -> - val currentSettings = settingsManager.currentSettings - val serverUrl = currentSettings.serverUrl - if (serverUrl.isEmpty()) return@hostnameVerifier false + val leafCert = chain.first() + val fingerprint = leafCert.sha256Fingerprint() + val trusted = trustedFingerprint(configuredHost) + + if (trusted == null) { + // First-time connection: accept here. Enrollment happens in verify() + // from this session's own peer certificate, so it is bound to the + // verified hostname and the actual handshake — no shared + // cross-connection state to race or poison. + log(Log.INFO, "TOFU first contact for host=$configuredHost (enrollment pending hostname verification)") + return + } + + if (!fingerprint.equals(trusted, ignoreCase = true)) { + log(Log.ERROR, "TLS fingerprint mismatch for host=$configuredHost") + throw SelfSignedCertTrustManager.CertificateFingerprintMismatchException( + host = configuredHost, + expectedFingerprint = trusted, + actualFingerprint = fingerprint, + ) + } + } + + override fun getAcceptedIssuers(): Array = emptyArray() + + override fun verify(hostname: String, session: SSLSession): Boolean { + val url = serverUrl() + if (url.isEmpty()) return false + + return try { + val configuredHost = URI(url).host + val matches = hostname.equals(configuredHost, ignoreCase = true) + if (!matches) return false + // Enroll TOFU here, from THIS session's own peer certificate, only + // if nothing is stored yet. Deriving the fingerprint from the + // verified session (rather than a shared field set during + // checkServerTrusted) ties it to this exact handshake and hostname, + // so concurrent first-time handshakes can't enroll each other's cert. + // Wrapped separately so an enrollment hiccup never blocks a valid connection. try { - val configuredHost = URI(serverUrl).host - val matches = hostname.equals(configuredHost, ignoreCase = true) - if (!matches) return@hostnameVerifier false - - // Enroll TOFU here, from THIS session's own peer certificate, only - // if nothing is stored yet. Deriving the fingerprint from the - // verified session (rather than a shared field set during - // checkServerTrusted) ties it to this exact handshake and hostname, - // so concurrent first-time handshakes can't enroll each other's cert. - // Wrapped separately so an enrollment hiccup never blocks a valid connection. - try { - val normalizedHost = configuredHost?.lowercase() - if (!normalizedHost.isNullOrEmpty() && - settingsManager.getTrustedCertificateFingerprint(normalizedHost) == null - ) { - val leaf = session.peerCertificates.firstOrNull() as? X509Certificate - if (leaf != null) { - settingsManager.saveTrustedCertificateFingerprint( - normalizedHost, - leaf.sha256Fingerprint(), - ) - Log.i(TAG, "TOFU enrolled fingerprint for host=$normalizedHost from verified session") - } + val normalizedHost = configuredHost?.lowercase() + if (!normalizedHost.isNullOrEmpty() && trustedFingerprint(normalizedHost) == null) { + val leaf = session.peerCertificates.firstOrNull() as? X509Certificate + if (leaf != null) { + saveFingerprint(normalizedHost, leaf.sha256Fingerprint()) + log(Log.INFO, "TOFU enrolled fingerprint for host=$normalizedHost from verified session") } - } catch (e: Exception) { - Log.w(TAG, "TOFU enrollment skipped: ${e.message}") } - - true - } catch (_: Exception) { - false + } catch (e: Exception) { + log(Log.WARN, "TOFU enrollment skipped: ${e.message}") } - } - return this + true + } catch (_: Exception) { + false + } } - private fun X509Certificate.sha256Fingerprint(): String { - val digest = MessageDigest.getInstance("SHA-256").digest(encoded) - return digest.joinToString(separator = ":") { "%02X".format(it) } + private companion object { + const val TAG = "SelfSignedTrustManager" } } + +private fun X509Certificate.sha256Fingerprint(): String { + val digest = MessageDigest.getInstance("SHA-256").digest(encoded) + return digest.joinToString(separator = ":") { "%02X".format(it) } +} diff --git a/app/src/test/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustTest.kt b/app/src/test/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustTest.kt new file mode 100644 index 00000000..4bd4608d --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustTest.kt @@ -0,0 +1,188 @@ +package com.ninelivesaudio.app.data.remote + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Assert.fail +import org.junit.Test +import java.io.ByteArrayInputStream +import java.lang.reflect.Proxy +import java.security.cert.CertificateException +import java.security.cert.CertificateFactory +import java.security.cert.X509Certificate +import javax.net.ssl.HostnameVerifier +import javax.net.ssl.SSLSession +import javax.net.ssl.X509TrustManager + +/** + * The self-signed toggle used to be read once, when Hilt built the OkHttp + * client, which is before settings load. So it was always off and flipping it + * did nothing. The trust manager and hostname verifier are now always + * installed and read the pref on every handshake. + * + * Off must be exactly the platform check. On keeps trust-on-first-use. + */ +class SelfSignedCertTrustTest { + + private var allow = false + private val fingerprints = mutableMapOf() + private val tofu = TofuServerTrust( + serverUrl = { "https://abs.local:13378" }, + trustedFingerprint = { fingerprints[it] }, + saveFingerprint = { host, fp -> fingerprints[host] = fp }, + log = { _, _ -> }, + ) + + private fun gatedTrust(platform: X509TrustManager = SelfSignedCertTrustManager.platformTrustManager()) = + PrefGatedTrustManager(platform, { allow }, tofu) + + private fun gatedVerifier(platform: HostnameVerifier) = + PrefGatedHostnameVerifier(platform, { allow }, tofu) + + // ─── Pref off: the platform decides, nothing else ───────────────────── + + @Test + fun `off rejects a self-signed cert the platform rejects`() { + allow = false + try { + gatedTrust().checkServerTrusted(arrayOf(certA), "RSA") + fail("platform trust accepted a self-signed cert") + } catch (_: CertificateException) { + } + assertTrue("off must never enroll a fingerprint", fingerprints.isEmpty()) + } + + @Test + fun `off hands the chain to the platform trust manager`() { + allow = false + val platform = RecordingTrustManager() + gatedTrust(platform).checkServerTrusted(arrayOf(certA), "RSA") + assertEquals(1, platform.serverChecks) + } + + @Test + fun `off hostname verification is the platform verifier's answer`() { + allow = false + var consulted = 0 + val platform = HostnameVerifier { _, _ -> consulted++; false } + // The host matches the configured server, which TOFU would accept. + assertFalse(gatedVerifier(platform).verify("abs.local", session(certA))) + assertEquals(1, consulted) + assertTrue(fingerprints.isEmpty()) + } + + // ─── Pref on: trust on first use, as before ─────────────────────────── + + @Test + fun `on accepts first contact and enrolls from the verified session`() { + allow = true + val verifier = gatedVerifier { _, _ -> fail("platform verifier used while on"); false } + gatedTrust().checkServerTrusted(arrayOf(certA), "RSA") + assertNull(fingerprints["abs.local"]) + assertTrue(verifier.verify("abs.local", session(certA))) + assertTrue(fingerprints["abs.local"] != null) + + // Same cert again passes, a different one is refused. + gatedTrust().checkServerTrusted(arrayOf(certA), "RSA") + try { + gatedTrust().checkServerTrusted(arrayOf(certB), "RSA") + fail("a rotated cert was accepted") + } catch (e: SelfSignedCertTrustManager.CertificateFingerprintMismatchException) { + assertEquals("abs.local", e.host) + } + } + + @Test + fun `on refuses a host other than the configured server`() { + allow = true + val verifier = gatedVerifier { _, _ -> true } + assertFalse(verifier.verify("evil.example", session(certA))) + assertTrue(fingerprints.isEmpty()) + } + + @Test + fun `flipping the pref takes effect on the next handshake`() { + val trust = gatedTrust() + allow = true + trust.checkServerTrusted(arrayOf(certA), "RSA") + allow = false + try { + trust.checkServerTrusted(arrayOf(certA), "RSA") + fail("off still trusted a self-signed cert") + } catch (e: CertificateException) { + assertFalse(e is SelfSignedCertTrustManager.CertificateFingerprintMismatchException) + } + } + + // ─── Fixtures ───────────────────────────────────────────────────────── + + private class RecordingTrustManager : X509TrustManager { + var serverChecks = 0 + override fun checkClientTrusted(chain: Array?, authType: String?) = Unit + override fun checkServerTrusted(chain: Array?, authType: String?) { + serverChecks++ + } + override fun getAcceptedIssuers(): Array = emptyArray() + } + + private fun session(leaf: X509Certificate): SSLSession = + Proxy.newProxyInstance( + SSLSession::class.java.classLoader, + arrayOf(SSLSession::class.java), + ) { _, method, _ -> + if (method.name == "getPeerCertificates") arrayOf(leaf) else null + } as SSLSession + + private fun parse(pem: String): X509Certificate = + CertificateFactory.getInstance("X.509") + .generateCertificate(ByteArrayInputStream(pem.toByteArray())) as X509Certificate + + // Two throwaway self-signed certs for abs.local, valid for a century. + private val certA = parse( + """ +-----BEGIN CERTIFICATE----- +MIIDITCCAgmgAwIBAgIUY7DYR9G8X1FVxBIffvHKCh/Wxs0wDQYJKoZIhvcNAQEL +BQAwFDESMBAGA1UEAwwJYWJzLmxvY2FsMCAXDTI2MDkzMDE1MTUwNFoYDzIxMjYw +OTA2MTUxNTA0WjAUMRIwEAYDVQQDDAlhYnMubG9jYWwwggEiMA0GCSqGSIb3DQEB +AQUAA4IBDwAwggEKAoIBAQC8WBXuIl1atgC1vz023dv5dr8nPZ2BacXKHMjwCdQu +ev3j8hnvNwahjs5c3XQfdlj6/OfjiianAnRjwk6Uy9wRuiz+jyzEWu+5sQXIylkC +ZD01xuwMPyUeu3WtWHWJ0PPJMMCwY5HmsFX5h9CImF3vzL/iDs6WDe37bz3rH1EL +0mM6Yjz30QPicBRo7Rx1m+ON5rds6XOhQgYk91br3CyN26Bs9EBCiTMpeM3UuRSn ++c09v0ot18rll7IpGv2r6n1QVEd5ZuAe/Ez+Hd6W4uYXzLyRwlVd5+sUSzI3tG3n +4negv0oZh6Mi8LrEUE98hpr6a7lMDGTBQX/ZY06tE/GvAgMBAAGjaTBnMB0GA1Ud +DgQWBBTWlonZoMqC8e8IQ836jcYxFCgB9DAfBgNVHSMEGDAWgBTWlonZoMqC8e8I +Q836jcYxFCgB9DAPBgNVHRMBAf8EBTADAQH/MBQGA1UdEQQNMAuCCWFicy5sb2Nh +bDANBgkqhkiG9w0BAQsFAAOCAQEAZKmXSBsUQrmODcuBS/SVODeSQRfln6ZUZ0DM +g5eEhM2YVUvd4saS+0F8XDaNXpBTF4FadKnFtrr+41UBOxCDhwrO5R55h6fLMu/f +KDs743Xpn2IgigakbXHmykH1IHYXLkiUN42PAaWIePZVk5mvO85jBySkUeX9Qqoh +v5O16RxEhbHiUMreI3uMxDbmhO7eoouu2jgQovsFmuGL6Va2UxM5UXBwQThek0cD +uibe0PQU56DbCZ9k8Eq7jZ/FM/oIgK/EZtwQlXLZghhAiffO7UANgxngkbF2OdOx +p6hD2l//jI7FJdtY/EO37Z+Hf0EydLDZgVAy3uK/b4jxtqa0Pg== +-----END CERTIFICATE----- +""" + ) + private val certB = parse( + """ +-----BEGIN CERTIFICATE----- +MIIDITCCAgmgAwIBAgIUe8FlwdtcSY87wnKjLniPbUgWfEkwDQYJKoZIhvcNAQEL +BQAwFDESMBAGA1UEAwwJYWJzLmxvY2FsMCAXDTI2MDkzMDE1MTUwNFoYDzIxMjYw +OTA2MTUxNTA0WjAUMRIwEAYDVQQDDAlhYnMubG9jYWwwggEiMA0GCSqGSIb3DQEB +AQUAA4IBDwAwggEKAoIBAQCQK/c9BR5tsS04m11Q1yXoYOQvmBd0x1je9F8h4mDq +OaHwb+fPUF9lNpfMvOPKp+RWCInrx1FMIkTg2pjitFNFJtAHNvrQCcT2TQaKaEM+ +28kpWEKasHRu/RuzH3rgypAYPocdyBCLHeb6FACW/6au1jHVJR/vrN37MJECsXxT +Eo3+19MC7OduLQJYrLr0SQdnnjUUhMjun74GcZUiB+asJdCnTKFmj/K0Enn0E5BM +3qa9/WJhORBh7Oz7/LbuvdbBQci9Tw27k7qZvSIw4k275FYhjUSiakZFPumW+Aoj +gWYJB7y45a0ADZvBbvQvTDCxl+qYy/OaJKMyodoxVj6tAgMBAAGjaTBnMB0GA1Ud +DgQWBBQIW7EUTneF8loAfry9OkXK9tQLxTAfBgNVHSMEGDAWgBQIW7EUTneF8loA +fry9OkXK9tQLxTAPBgNVHRMBAf8EBTADAQH/MBQGA1UdEQQNMAuCCWFicy5sb2Nh +bDANBgkqhkiG9w0BAQsFAAOCAQEAiNS5TrsnBHl7bxYrYslPVwP2oBd2i84GGmg0 +Gkwd3HUOy6R3iHKciL0tBEwFqzoWuQqo/24FWUzSiWkvzDMUFNsXbvLhZanWrKcc +bB4Z7dXNYK534oHNh7HKDVbMFYJWCiJd4ZSkRfl0mtoakPfG0Qjibs3SvWbFnylI +VJh4fBArILd8gium7FknT4pY/WVx3jJCU41x81vX+lKJANOm2w62mOtYpakwTWHp +XlhEHzni3xcxAIBFU55tgfN26nQC+upQ4e5iQFH3ZLJhq1FPFbMeiE8UeaXTN+8z +IpB+oqHjOBILWwuDVxRYaR1fDrFnzmVmweG+IyjAZZD2WXGY+g== +-----END CERTIFICATE----- +""" + ) +} From 1663e147ea1ecc8bc5d8554937e3e2c17523fdab Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:17:24 -0400 Subject: [PATCH 04/16] Play right after a download finishes uses the downloaded copy When a download finished while its detail screen was open, the screen flipped to Downloaded but kept the book it loaded before the download, which has no local files. Pressing Play then streamed from the server, or with no connection refused with "You are offline", until you left and came back. The screen now reloads the saved book when its download completes, the same reload it does on returning to the screen. Co-Authored-By: Claude Opus 5.5 --- .../ninelivesaudio/app/ui/bookdetail/BookDetailViewModel.kt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/bookdetail/BookDetailViewModel.kt b/app/src/main/java/com/ninelivesaudio/app/ui/bookdetail/BookDetailViewModel.kt index 1746d742..b17f8fd1 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/bookdetail/BookDetailViewModel.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/bookdetail/BookDetailViewModel.kt @@ -286,6 +286,9 @@ class BookDetailViewModel @Inject constructor( downloadProgress = 100, ) } + // The held book predates the download and has no local path, + // so Play would stream it (or refuse offline). Reload it. + loadBook() } } } From 519417f91394d008fec247738dd23cdc3520db01 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:19:07 -0400 Subject: [PATCH 05/16] Downloaded books start playing without waiting on the server Pressing Play on a downloaded book first opened a listening session on the server, with the normal 30 second connect and 60 second read timeouts. With the server gone that meant a spinner for up to a minute or more before a book sitting on the phone started playing. A downloaded book now skips that step when the app already knows the server is unreachable, and waits at most 5 seconds for it otherwise. The session is still opened, in the background once playback starts, which is the path restored playback already uses. Streaming books are unchanged. One behavior change: when the session is skipped or late, the start position comes from synced progress (the book's saved position and local progress) instead of the position the server session reports. Those normally agree, since progress sync keeps them in step. Co-Authored-By: Claude Opus 5.5 --- .../app/service/PlaybackManager.kt | 33 ++++++++++++++++- .../app/service/DownloadedSessionOpenTest.kt | 35 +++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 app/src/test/java/com/ninelivesaudio/app/service/DownloadedSessionOpenTest.kt diff --git a/app/src/main/java/com/ninelivesaudio/app/service/PlaybackManager.kt b/app/src/main/java/com/ninelivesaudio/app/service/PlaybackManager.kt index 46067f77..611a3dac 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/PlaybackManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/PlaybackManager.kt @@ -559,6 +559,31 @@ internal fun listeningSessionToOpen( else -> ListeningSessionKind.NONE } +internal enum class LoadSessionOpen { AWAIT, BOUNDED, DEFER } + +/** How long a downloaded book's load waits on its server listening session. */ +internal val DOWNLOADED_SESSION_OPEN_TIMEOUT = 5.seconds + +/** + * How a load opens its server listening session. + * + * A streaming book waits for it, because its tracks and position feed the + * load. A downloaded book plays from disk, so it skips the open while the + * server is not reachable and waits at most [DOWNLOADED_SESSION_OPEN_TIMEOUT] + * while it is. Either way ensureListeningSessionStarted opens the session in + * the background once playback starts. SYNCING counts as reachable, as it + * does everywhere else. + */ +internal fun loadSessionOpen( + usesLocalTracks: Boolean, + connectionStatus: ConnectivityMonitor.ConnectionStatus, +): LoadSessionOpen = when { + !usesLocalTracks -> LoadSessionOpen.AWAIT + connectionStatus == ConnectivityMonitor.ConnectionStatus.CONNECTED || + connectionStatus == ConnectivityMonitor.ConnectionStatus.SYNCING -> LoadSessionOpen.BOUNDED + else -> LoadSessionOpen.DEFER +} + internal enum class PlaybackItemPersistence { KEEP, SAVE, CLEAR } internal fun playbackItemPersistenceAction( @@ -1273,7 +1298,13 @@ class PlaybackManager @Inject constructor( hasLocalSession = synchronized(sessionLock) { currentLocalSessionId != null }, ) == ListeningSessionKind.SERVER ) { - openServerListeningSession(book, requestedGeneration, loadRequest) + when (loadSessionOpen(remoteAccess.usesLocalTracks(), connectivityMonitor.connectionStatus.value)) { + LoadSessionOpen.AWAIT -> openServerListeningSession(book, requestedGeneration, loadRequest) + LoadSessionOpen.BOUNDED -> withTimeoutOrNull(DOWNLOADED_SESSION_OPEN_TIMEOUT) { + openServerListeningSession(book, requestedGeneration, loadRequest) + } + LoadSessionOpen.DEFER -> Unit + } if (!playbackLoadOwner.isCurrent(loadRequest)) return false } diff --git a/app/src/test/java/com/ninelivesaudio/app/service/DownloadedSessionOpenTest.kt b/app/src/test/java/com/ninelivesaudio/app/service/DownloadedSessionOpenTest.kt new file mode 100644 index 00000000..ab1c3bbe --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/service/DownloadedSessionOpenTest.kt @@ -0,0 +1,35 @@ +package com.ninelivesaudio.app.service + +import com.ninelivesaudio.app.service.ConnectivityMonitor.ConnectionStatus +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * A downloaded book plays from disk, so its load must not sit on the main + * client's 30s connect timeout waiting for a listening session from a server + * that is not there. Streaming books still wait, because the session feeds + * their load. + */ +class DownloadedSessionOpenTest { + + @Test + fun `a streaming book waits for its session whatever the connection says`() { + ConnectionStatus.entries.forEach { status -> + assertEquals(LoadSessionOpen.AWAIT, loadSessionOpen(usesLocalTracks = false, connectionStatus = status)) + } + } + + @Test + fun `a downloaded book skips the session while the server is not reachable`() { + listOf(ConnectionStatus.OFFLINE, ConnectionStatus.SERVER_UNREACHABLE).forEach { status -> + assertEquals(LoadSessionOpen.DEFER, loadSessionOpen(usesLocalTracks = true, connectionStatus = status)) + } + } + + @Test + fun `a downloaded book waits only briefly on a live server`() { + listOf(ConnectionStatus.CONNECTED, ConnectionStatus.SYNCING).forEach { status -> + assertEquals(LoadSessionOpen.BOUNDED, loadSessionOpen(usesLocalTracks = true, connectionStatus = status)) + } + } +} From 906b7e35fd84d0f68772ee7528d08a4e88b9788b Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:45:47 -0400 Subject: [PATCH 06/16] A deleted download can be downloaded again from its book page downloadCompleted replays its last event to new collectors. The book page trusted that event and forced the Downloaded state, so reopening a book whose download had just been deleted showed Downloaded with no way to re-download it until the app restarted. The page now reloads the stored book on a completion event and lets that row decide. This was live in 2.1.4. Co-Authored-By: Claude Opus 5.5 --- .../app/ui/bookdetail/BookDetailViewModel.kt | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/bookdetail/BookDetailViewModel.kt b/app/src/main/java/com/ninelivesaudio/app/ui/bookdetail/BookDetailViewModel.kt index b17f8fd1..b3f1179b 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/bookdetail/BookDetailViewModel.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/bookdetail/BookDetailViewModel.kt @@ -279,15 +279,11 @@ class BookDetailViewModel @Inject constructor( viewModelScope.launch { downloadManager.downloadCompleted.collect { completedItem -> if (completedItem.audioBookId == bookId) { - _uiState.update { - it.copy( - isDownloaded = true, - downloadState = DownloadButtonState.COMPLETED, - downloadProgress = 100, - ) - } // The held book predates the download and has no local path, - // so Play would stream it (or refuse offline). Reload it. + // so Play would stream it (or refuse offline). Reload it and + // let the stored row decide. This flow replays its last event + // to new collectors, so trusting the event itself marked a + // book whose download was since deleted as downloaded. loadBook() } } From 3b26664b71aae563bd27ea710364b3742aea1e13 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:50:18 -0400 Subject: [PATCH 07/16] Switching books no longer waits 30s on a server that is not there With the server down but a VPN keeping the phone "online", stopping one book and starting a downloaded one sat for the full 30 second connect timeout. The next load waits on the old book's final progress write, and that write was trying the server before letting go. The final position still lands on the phone first, along with its offline queue row. The server push now runs only while the server is known to be live, and gives up after 5 seconds. Either way the queued row goes out on the next reconnect or periodic flush. Co-Authored-By: Claude Opus 5.5 --- .../app/data/repository/ProgressRepository.kt | 21 +++++-- .../ninelivesaudio/app/service/SyncManager.kt | 23 +++++++- .../app/service/TerminalProgressPushTest.kt | 56 +++++++++++++++++++ 3 files changed, 93 insertions(+), 7 deletions(-) create mode 100644 app/src/test/java/com/ninelivesaudio/app/service/TerminalProgressPushTest.kt diff --git a/app/src/main/java/com/ninelivesaudio/app/data/repository/ProgressRepository.kt b/app/src/main/java/com/ninelivesaudio/app/data/repository/ProgressRepository.kt index 383dcc63..62f000bf 100644 --- a/app/src/main/java/com/ninelivesaudio/app/data/repository/ProgressRepository.kt +++ b/app/src/main/java/com/ninelivesaudio/app/data/repository/ProgressRepository.kt @@ -14,6 +14,7 @@ import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeoutOrNull import javax.inject.Inject import javax.inject.Singleton import kotlin.time.Duration @@ -320,6 +321,7 @@ class ProgressRepository @Inject constructor( isFinished: Boolean, duration: Double, pushToServer: Boolean, + pushTimeout: Duration = Duration.INFINITE, onPersisted: suspend () -> Unit = {}, ): Boolean = pendingProgressQueueOwner.withItemLock(itemId) { persistProgressAndEnqueueLocked( @@ -330,12 +332,15 @@ class ProgressRepository @Inject constructor( onPersisted = onPersisted, ) val pushed = if (pushToServer && progressCanBeDelivered(isFinished, duration)) { - try { - apiService.updateProgress(itemId, currentTime, isFinished, duration) - } catch (cancellation: kotlinx.coroutines.CancellationException) { - throw cancellation - } catch (_: Exception) { - false + // The row queued above delivers it later if this gives up. + pushWithin(pushTimeout) { + try { + apiService.updateProgress(itemId, currentTime, isFinished, duration) + } catch (cancellation: kotlinx.coroutines.CancellationException) { + throw cancellation + } catch (_: Exception) { + false + } } } else { false @@ -619,6 +624,10 @@ internal fun serverProgressMayReplaceLocal(hasPendingProgress: Boolean): Boolean * from their durable source of truth by [legacyProgressSnapshot], so this check * only decides the fate of rows with proven atomic provenance. */ +/** A push that has not answered within [timeout] counts as not delivered. */ +internal suspend fun pushWithin(timeout: Duration, push: suspend () -> Boolean): Boolean = + withTimeoutOrNull(timeout) { push() } ?: false + internal fun queuedRowsAreSuperseded( rows: List, durableProgress: PlaybackProgressEntity?, diff --git a/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt b/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt index 8fd114ca..0b94c18f 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt @@ -472,7 +472,11 @@ class SyncManager @Inject constructor( currentTime = safeCurrentTime, isFinished = computedFinished, duration = safeDuration, - pushToServer = connectivityMonitor.isOnline.value, + pushToServer = terminalProgressPushAllowed( + isOnline = connectivityMonitor.isOnline.value, + connectionStatus = connectivityMonitor.connectionStatus.value, + ), + pushTimeout = TERMINAL_PROGRESS_PUSH_TIMEOUT, onPersisted = onPersisted, ) @@ -570,6 +574,23 @@ internal fun shouldReconnectForModeTransition( newMode: AppMode, ): Boolean = previousMode != AppMode.AUDIOBOOKSHELF && newMode == AppMode.AUDIOBOOKSHELF +/** How long a stopped book's final progress push waits on the server. */ +internal val TERMINAL_PROGRESS_PUSH_TIMEOUT = 5.seconds + +/** + * Whether a stopped book's final progress tries the server now. The next + * book's load waits on this push, and a VPN can keep the OS online while the + * server is gone, so only a live server gets the attempt. The local write and + * its offline queue row land first either way, and the reconnect flush + * delivers the row. + */ +internal fun terminalProgressPushAllowed( + isOnline: Boolean, + connectionStatus: ConnectivityMonitor.ConnectionStatus, +): Boolean = isOnline && + (connectionStatus == ConnectivityMonitor.ConnectionStatus.CONNECTED || + connectionStatus == ConnectivityMonitor.ConnectionStatus.SYNCING) + /** * Gate for a server sync. Requires an authenticated, non-LOCAL session AND an * active network. The online check is the "internet connection check" that diff --git a/app/src/test/java/com/ninelivesaudio/app/service/TerminalProgressPushTest.kt b/app/src/test/java/com/ninelivesaudio/app/service/TerminalProgressPushTest.kt new file mode 100644 index 00000000..e80bdfc5 --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/service/TerminalProgressPushTest.kt @@ -0,0 +1,56 @@ +package com.ninelivesaudio.app.service + +import com.ninelivesaudio.app.data.repository.pushWithin +import com.ninelivesaudio.app.service.ConnectivityMonitor.ConnectionStatus +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.seconds + +/** + * Stopping a book writes its final position locally, then pushes it to the + * server. The next book's load waits on that job, so a push to a server that + * is not answering (a VPN keeps the OS online) held a downloaded book for the + * full 30s connect timeout. The push now runs only while the server is live + * and gives up quickly. The offline queue row written first delivers it later. + */ +class TerminalProgressPushTest { + + @Test + fun `the push runs only while the server is live`() { + listOf(ConnectionStatus.CONNECTED, ConnectionStatus.SYNCING).forEach { status -> + assertTrue(terminalProgressPushAllowed(isOnline = true, connectionStatus = status)) + } + listOf(ConnectionStatus.OFFLINE, ConnectionStatus.SERVER_UNREACHABLE).forEach { status -> + assertFalse(terminalProgressPushAllowed(isOnline = true, connectionStatus = status)) + } + ConnectionStatus.entries.forEach { status -> + assertFalse(terminalProgressPushAllowed(isOnline = false, connectionStatus = status)) + } + } + + @Test + fun `the push waits about five seconds at most`() { + assertEquals(5.seconds, TERMINAL_PROGRESS_PUSH_TIMEOUT) + } + + @Test(timeout = 5_000) + fun `a push that never answers gives up at the bound`() = runBlocking { + val never = CompletableDeferred() + val started = System.nanoTime() + val pushed = pushWithin(100.milliseconds) { never.await() } + val elapsedMs = (System.nanoTime() - started) / 1_000_000 + assertFalse(pushed) + assertTrue("waited ${elapsedMs}ms", elapsedMs < 2_000) + } + + @Test + fun `a push that answers in time reports its result`() = runBlocking { + assertTrue(pushWithin(1.seconds) { true }) + assertFalse(pushWithin(1.seconds) { false }) + } +} From f2d491bd4e5534636c27d6cfc5e8e0947c95115b Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:52:21 -0400 Subject: [PATCH 08/16] Pause, cancel and delete finish even if you leave the Downloads screen Pausing, cancelling or deleting the book that is downloading stops the worker first, waits for it, then writes the change and restarts the queue. All of that ran inside the Downloads screen. Leaving the screen in the middle could stop the worker and skip the rest, so the book stayed Downloading and nothing else in the queue moved until the app restarted. The download manager now runs the whole action itself. The screen still waits for it, but leaving no longer cuts it short. Co-Authored-By: Claude Opus 5.5 --- .../app/service/DownloadManager.kt | 18 +++++-- .../app/service/download/DownloadWork.kt | 15 ++++++ .../download/UserDownloadOwnerScopeTest.kt | 49 +++++++++++++++++++ 3 files changed, 78 insertions(+), 4 deletions(-) create mode 100644 app/src/test/java/com/ninelivesaudio/app/service/download/UserDownloadOwnerScopeTest.kt diff --git a/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt b/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt index 2928739f..d1be084a 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt @@ -18,11 +18,14 @@ import com.ninelivesaudio.app.service.download.DownloadEngine import com.ninelivesaudio.app.service.download.DownloadNotifications import com.ninelivesaudio.app.service.download.DownloadQueueWorker import com.ninelivesaudio.app.service.download.estimateTotalBytes +import com.ninelivesaudio.app.service.download.finishInOwnerScope import com.ninelivesaudio.app.service.download.DownloadSlotStore import com.ninelivesaudio.app.service.download.selectNextDownload import com.ninelivesaudio.app.service.download.writeAfterEngineStops import dagger.hilt.android.qualifiers.ApplicationContext +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.SharedFlow import kotlinx.coroutines.flow.asSharedFlow @@ -71,6 +74,13 @@ class DownloadManager @Inject constructor( private val slotMutex = Mutex() private val workManager: WorkManager by lazy { WorkManager.getInstance(context) } + /** + * Where user pause, cancel and delete actually run. Their callers are screen + * coroutines, and leaving the screen between the worker stop and the row + * write used to leave the book Downloading with the queue stopped. + */ + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO) + private companion object { const val WORKER_STOP_TIMEOUT_MS = 10_000L const val WORKER_STOP_POLL_MS = 100L @@ -455,8 +465,8 @@ class DownloadManager @Inject constructor( } /** Pause a download: mark it Paused; restart the drain if it was the active one. */ - suspend fun pauseDownload(downloadId: String) { - val entity = downloadItemDao.getById(downloadId) ?: return + suspend fun pauseDownload(downloadId: String): Unit = finishInOwnerScope(scope) { + val entity = downloadItemDao.getById(downloadId) ?: return@finishInOwnerScope val wasDownloading = entity.status == DownloadStatus.Downloading.ordinal writeAfterEngineStops(wasDownloading, { stopDrainAndAwait() }) { // Re-read after the stop: the engine's last tick moved the bytes on, @@ -500,7 +510,7 @@ class DownloadManager @Inject constructor( } /** Cancel a download and clean up; restart the drain if it was the active one. */ - suspend fun cancelDownload(downloadId: String) { + suspend fun cancelDownload(downloadId: String): Unit = finishInOwnerScope(scope) { val entity = downloadItemDao.getById(downloadId) val wasDownloading = entity?.status == DownloadStatus.Downloading.ordinal writeAfterEngineStops(wasDownloading, { stopDrainAndAwait() }) { @@ -519,7 +529,7 @@ class DownloadManager @Inject constructor( } /** Delete a download's files and DB record. */ - suspend fun deleteDownload(audioBookId: String) { + suspend fun deleteDownload(audioBookId: String): Unit = finishInOwnerScope(scope) { val downloadEntity = downloadItemDao.getByAudioBookId(audioBookId) val wasDownloading = downloadEntity?.status == DownloadStatus.Downloading.ordinal writeAfterEngineStops(wasDownloading, { stopDrainAndAwait() }) { diff --git a/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt b/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt index 8793fb24..6d52c36c 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt @@ -2,6 +2,8 @@ package com.ninelivesaudio.app.service.download import com.ninelivesaudio.app.domain.model.DownloadItem import com.ninelivesaudio.app.domain.model.DownloadStatus +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.async // ─── WorkManager identifier + queue selection ───────────────────────────── // @@ -44,3 +46,16 @@ internal suspend fun writeAfterEngineStops( if (engineActive) stopEngine() return write() } + +/** + * Run a user's pause, cancel or delete in [owner] and await it. + * + * Callers are screen coroutines that die when the user navigates away. Once + * the engine has been stopped, the row write and the queue restart must still + * happen, or the book sits Downloading with the queue stopped. The caller can + * be cancelled and stop waiting, the operation itself carries on. + */ +internal suspend fun finishInOwnerScope( + owner: CoroutineScope, + operation: suspend () -> T, +): T = owner.async { operation() }.await() diff --git a/app/src/test/java/com/ninelivesaudio/app/service/download/UserDownloadOwnerScopeTest.kt b/app/src/test/java/com/ninelivesaudio/app/service/download/UserDownloadOwnerScopeTest.kt new file mode 100644 index 00000000..05977726 --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/service/download/UserDownloadOwnerScopeTest.kt @@ -0,0 +1,49 @@ +package com.ninelivesaudio.app.service.download + +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * Pause, cancel and delete from the Downloads screen run in the screen's + * ViewModel coroutine. Leaving the screen cancels it, and if that landed after + * the worker was stopped but before the row write, the book stayed Downloading + * with the queue stopped. The operation now runs in the manager's own scope, + * so the caller can leave without taking the write and restart with it. + */ +class UserDownloadOwnerScopeTest { + + @Test + fun `leaving the screen mid-operation still finishes the write`() = runBlocking { + val owner = CoroutineScope(SupervisorJob() + Dispatchers.Default) + val stopped = CompletableDeferred() + val release = CompletableDeferred() + val finished = CompletableDeferred() + + val caller = launch { + finishInOwnerScope(owner) { + stopped.complete(Unit) + release.await() + finished.complete("written and restarted") + } + } + stopped.await() + caller.cancelAndJoin() + release.complete(Unit) + + assertEquals("written and restarted", withTimeout(2_000) { finished.await() }) + } + + @Test + fun `a caller that stays gets the result`() = runBlocking { + val owner = CoroutineScope(SupervisorJob() + Dispatchers.Default) + assertEquals(7, finishInOwnerScope(owner) { 7 }) + } +} From d4177f7a72c2e4523991e1fdc2484c612f68fe78 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:53:15 -0400 Subject: [PATCH 09/16] Pausing a download at the very end no longer strands it A download marks itself finished a moment before the book is marked as on the phone, and saves the cover in between. Pausing in that gap left a download that said finished while the book had no local copy. Nothing would pick it back up, Resume refused it, and offline play never unlocked for that book. Pause now only leaves a finished download alone when the book really is on the phone. Otherwise it is paused like any other, and Resume finishes it without downloading the audio again. Co-Authored-By: Claude Opus 5.5 --- .../app/service/DownloadManager.kt | 10 ++++-- .../app/service/download/DownloadWork.kt | 12 +++++++ .../download/PauseNearCompletionTest.kt | 36 +++++++++++++++++++ 3 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 app/src/test/java/com/ninelivesaudio/app/service/download/PauseNearCompletionTest.kt diff --git a/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt b/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt index d1be084a..74a3d1b6 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/DownloadManager.kt @@ -19,6 +19,7 @@ import com.ninelivesaudio.app.service.download.DownloadNotifications import com.ninelivesaudio.app.service.download.DownloadQueueWorker import com.ninelivesaudio.app.service.download.estimateTotalBytes import com.ninelivesaudio.app.service.download.finishInOwnerScope +import com.ninelivesaudio.app.service.download.pauseKeepsCompletedRow import com.ninelivesaudio.app.service.download.DownloadSlotStore import com.ninelivesaudio.app.service.download.selectNextDownload import com.ninelivesaudio.app.service.download.writeAfterEngineStops @@ -470,9 +471,14 @@ class DownloadManager @Inject constructor( val wasDownloading = entity.status == DownloadStatus.Downloading.ordinal writeAfterEngineStops(wasDownloading, { stopDrainAndAwait() }) { // Re-read after the stop: the engine's last tick moved the bytes on, - // and it may have finished the book in the meantime. + // and it may have finished the book in the meantime. A Completed row + // only counts once the book itself is marked downloaded, since the + // stop can land between those two writes. val latest = downloadItemDao.getById(downloadId) ?: return@writeAfterEngineStops - if (latest.status == DownloadStatus.Completed.ordinal) return@writeAfterEngineStops + val book = audioBookDao.getById(latest.audioBookId) + if (pauseKeepsCompletedRow(latest.status, book?.isDownloaded, book?.localPath)) { + return@writeAfterEngineStops + } downloadItemDao.upsert(latest.copy(status = DownloadStatus.Paused.ordinal)) } if (wasDownloading) { diff --git a/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt b/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt index 6d52c36c..cca14123 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/download/DownloadWork.kt @@ -59,3 +59,15 @@ internal suspend fun finishInOwnerScope( owner: CoroutineScope, operation: suspend () -> T, ): T = owner.async { operation() }.await() + +/** + * Whether a pause should leave the row alone because the book really finished. + * + * The engine writes Completed before it marks the book downloaded, so a pause + * that cancels it in between leaves a Completed row with no local copy. Only a + * book row with [isDownloaded] set and a [localPath] proves the finish. Anything + * short of that gets Paused, and resume re-runs the engine (it skips files it + * already has) to finish the job. + */ +internal fun pauseKeepsCompletedRow(rowStatus: Int, isDownloaded: Int?, localPath: String?): Boolean = + rowStatus == DownloadStatus.Completed.ordinal && isDownloaded == 1 && !localPath.isNullOrEmpty() diff --git a/app/src/test/java/com/ninelivesaudio/app/service/download/PauseNearCompletionTest.kt b/app/src/test/java/com/ninelivesaudio/app/service/download/PauseNearCompletionTest.kt new file mode 100644 index 00000000..4434a29b --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/service/download/PauseNearCompletionTest.kt @@ -0,0 +1,36 @@ +package com.ninelivesaudio.app.service.download + +import com.ninelivesaudio.app.domain.model.DownloadStatus +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The engine writes the Completed download row before it marks the book + * downloaded, and fetches the cover in between. A pause that lands there + * cancels the cover fetch, so the row says Completed while the book has no + * local copy. Pause used to leave any Completed row alone, which stranded it: + * the worker skips it, Resume rejects it, and offline play never unlocks. + */ +class PauseNearCompletionTest { + + @Test + fun `a finished download is left alone`() { + assertTrue( + pauseKeepsCompletedRow(DownloadStatus.Completed.ordinal, isDownloaded = 1, localPath = "/books/a"), + ) + } + + @Test + fun `a completed row without a downloaded book is paused so resume finishes it`() { + assertFalse(pauseKeepsCompletedRow(DownloadStatus.Completed.ordinal, isDownloaded = 0, localPath = null)) + assertFalse(pauseKeepsCompletedRow(DownloadStatus.Completed.ordinal, isDownloaded = 1, localPath = null)) + assertFalse(pauseKeepsCompletedRow(DownloadStatus.Completed.ordinal, isDownloaded = 1, localPath = "")) + assertFalse(pauseKeepsCompletedRow(DownloadStatus.Completed.ordinal, isDownloaded = null, localPath = null)) + } + + @Test + fun `a row still downloading is paused`() { + assertFalse(pauseKeepsCompletedRow(DownloadStatus.Downloading.ordinal, isDownloaded = 1, localPath = "/books/a")) + } +} From 9400ef7a20b29e7dc47168df48f16bd4fc88b420 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:54:56 -0400 Subject: [PATCH 10/16] An empty Library fills in when the server comes back Starting the app offline with no saved library list left nothing selected. When the server came back, the background sync cleared the "Last sync failed" banner, but the screen only re-filtered a shelf that had no library behind it, so it stayed empty until a manual refresh. When a sync brings data back and nothing is selected, or the list of libraries changed, the Library now reloads its libraries and picks one, the same way it does on launch. Co-Authored-By: Claude Opus 5.5 --- .../app/ui/library/LibraryViewModel.kt | 32 ++++++++++++++- .../ui/library/LibraryReconnectReloadTest.kt | 40 +++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryReconnectReloadTest.kt diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt index 2c61e42f..09abca97 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt @@ -99,6 +99,20 @@ internal fun shouldRequeryShelfAfterSync( record.outcomeSequence != previousSequence && record.result != SyncResult.FAILED +/** + * Whether a sync that brought data back needs the library list and selection + * reloaded, not just the shelf re-filtered. Re-filtering with nothing selected + * is a no-op, so an offline cold start with no saved libraries stayed empty + * after the server returned. An empty cache with nothing shown stays put, or a + * server with no libraries would reload on every sync record it writes. + */ +internal fun shouldReloadLibrariesAfterSync( + selectedLibrary: Library?, + shownLibraryIds: List, + cachedLibraryIds: List, +): Boolean = (selectedLibrary == null && cachedLibraryIds.isNotEmpty()) || + cachedLibraryIds != shownLibraryIds + internal fun decideDownloadedOnlyFilter( previousStatus: ConnectionStatus, newStatus: ConnectionStatus, @@ -271,7 +285,23 @@ class LibraryViewModel @Inject constructor( } // A background sync (reconnect or periodic) wrote new rows. // Re-query so the shelf shows them, not just a cleared banner. - if (shouldRequeryShelfAfterSync(previousSequence, record)) applyFilter() + if (shouldRequeryShelfAfterSync(previousSequence, record)) { + val state = _uiState.value + val cached = visibleCachedLibraries( + settings = settingsManager.currentSettings, + cached = libraryRepository.getAudiobookshelf(), + ) + if (shouldReloadLibrariesAfterSync( + selectedLibrary = state.selectedLibrary, + shownLibraryIds = state.libraries.map { it.id }, + cachedLibraryIds = cached.map { it.id }, + ) + ) { + libraryLoadLaunch.launch(viewModelScope) { loadLibraries() } + } else { + applyFilter() + } + } } } diff --git a/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryReconnectReloadTest.kt b/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryReconnectReloadTest.kt new file mode 100644 index 00000000..8566f46c --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryReconnectReloadTest.kt @@ -0,0 +1,40 @@ +package com.ninelivesaudio.app.ui.library + +import com.ninelivesaudio.app.domain.model.Library +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * A fresh offline start with no saved library list has nothing selected, and + * re-filtering a shelf with no library does nothing. So when the server came + * back, the background sync cleared the failure banner and the Library stayed + * empty. A sync that brings libraries back while nothing is selected, or that + * changes the library list, now reloads the libraries and the selection. + */ +class LibraryReconnectReloadTest { + + private val books = Library(id = "books") + private val podcasts = Library(id = "podcasts") + + @Test + fun `nothing selected and libraries came back reloads the list`() { + assertTrue(shouldReloadLibrariesAfterSync(null, emptyList(), listOf("books"))) + } + + @Test + fun `a changed library list reloads the list`() { + assertTrue(shouldReloadLibrariesAfterSync(books, listOf("books"), listOf("books", "podcasts"))) + assertTrue(shouldReloadLibrariesAfterSync(books, listOf("books", "podcasts"), listOf("books"))) + } + + @Test + fun `same libraries with one selected only refilters`() { + assertFalse(shouldReloadLibrariesAfterSync(podcasts, listOf("books", "podcasts"), listOf("books", "podcasts"))) + } + + @Test + fun `a server with no libraries does not reload forever`() { + assertFalse(shouldReloadLibrariesAfterSync(null, emptyList(), emptyList())) + } +} From e603e5e1fce66c72a82d453144fef68bf32fd750 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:55:45 -0400 Subject: [PATCH 11/16] "Nothing Downloaded" only shows when nothing is downloaded When the server drops, the Library switches to downloaded books, and an empty shelf said "Nothing Downloaded". It said that even when downloads existed but Hide Finished, the In Progress or Completed tab, a series or author pick, or a search was hiding them. That sent people looking for books that were on the phone the whole time. The message now only appears when no other filter is narrowing the shelf. Otherwise the usual "Try adjusting your filters" copy shows. Co-Authored-By: Claude Opus 5.5 --- .../app/ui/library/LibraryScreen.kt | 9 ++++++++- .../ui/library/LibraryOfflineEmptyCopyTest.kt | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryScreen.kt b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryScreen.kt index 7997beb5..e897e546 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryScreen.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryScreen.kt @@ -1155,10 +1155,17 @@ internal fun syncWarningMessage(result: SyncResult, showingSavedBooks: Boolean): /** * The server dropped, the shelf auto-switched to downloaded books, and there * are none. Saved streamed books exist but cannot play, so say that instead - * of blaming a filter the user never touched. + * of blaming a filter the user never touched. Only when no other filter is + * narrowing the shelf, or downloads hidden by Hide Finished, a tab, a group + * or a search would be reported as missing. The Downloaded tab is the same + * filter, so it counts as unfiltered here. */ internal fun isOfflineDownloadedOnlyEmpty(uiState: LibraryViewModel.UiState): Boolean = uiState.showDownloadedOnly && uiState.totalBookCount > 0 && + !uiState.hideFinished && + (uiState.selectedTab == LibraryTab.All || uiState.selectedTab == LibraryTab.Downloaded) && + uiState.selectedGroupFilter == null && + uiState.searchQuery.isBlank() && uiState.connectionStatus != com.ninelivesaudio.app.service.ConnectivityMonitor.ConnectionStatus.CONNECTED && uiState.connectionStatus != com.ninelivesaudio.app.service.ConnectivityMonitor.ConnectionStatus.SYNCING diff --git a/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryOfflineEmptyCopyTest.kt b/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryOfflineEmptyCopyTest.kt index 8c1602f7..653bd6bc 100644 --- a/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryOfflineEmptyCopyTest.kt +++ b/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryOfflineEmptyCopyTest.kt @@ -35,6 +35,22 @@ class LibraryOfflineEmptyCopyTest { assertFalse(isOfflineDownloadedOnlyEmpty(state(total = 0))) } + @Test + fun `another filter hiding the downloads keeps the filter copy`() { + assertFalse(isOfflineDownloadedOnlyEmpty(state().copy(hideFinished = true))) + assertFalse(isOfflineDownloadedOnlyEmpty(state().copy(selectedTab = LibraryTab.InProgress))) + assertFalse(isOfflineDownloadedOnlyEmpty(state().copy(selectedTab = LibraryTab.Completed))) + assertFalse(isOfflineDownloadedOnlyEmpty(state().copy(selectedGroupFilter = "Dune"))) + assertFalse(isOfflineDownloadedOnlyEmpty(state().copy(searchQuery = "dune"))) + } + + @Test + fun `the Downloaded tab and plain grouping still say nothing is downloaded`() { + assertTrue(isOfflineDownloadedOnlyEmpty(state().copy(selectedTab = LibraryTab.Downloaded))) + assertTrue(isOfflineDownloadedOnlyEmpty(state().copy(viewMode = ViewMode.SERIES))) + assertTrue(isOfflineDownloadedOnlyEmpty(state().copy(searchQuery = " "))) + } + @Test fun `banner only claims saved books when the shelf shows some`() { assertEquals("Last sync failed. Showing saved books.", syncWarningMessage(SyncResult.FAILED, true)) From 78000a4d19009abdfdb63d7d503462e53c726551 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 11:57:31 -0400 Subject: [PATCH 12/16] Switching books no longer waits on closing a dead server's session Stopping a book saves its final position on the phone, then sends the listening session's last update to the server and closes it. The next book waits for all of that. With the server gone mid-listen, those two server calls each sat for the full 30 second connect timeout, even after the position push itself was fixed to give up quickly. Both calls now follow the same rule as the position push: they only run while the server is known to be live, and give up after 5 seconds. The position on the phone is saved first, as before. A session that never got its close stays open on the server until the server expires it, which only affects the server's listening stats, not your progress. Co-Authored-By: Claude Opus 5.5 --- .../app/service/PlaybackManager.kt | 44 ++++++++++----- .../ninelivesaudio/app/service/SyncManager.kt | 13 +++++ .../app/service/TerminalSessionCallTest.kt | 54 +++++++++++++++++++ 3 files changed, 99 insertions(+), 12 deletions(-) create mode 100644 app/src/test/java/com/ninelivesaudio/app/service/TerminalSessionCallTest.kt diff --git a/app/src/main/java/com/ninelivesaudio/app/service/PlaybackManager.kt b/app/src/main/java/com/ninelivesaudio/app/service/PlaybackManager.kt index 611a3dac..a461c06e 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/PlaybackManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/PlaybackManager.kt @@ -2055,7 +2055,7 @@ class PlaybackManager @Inject constructor( try { syncTerminalSession(terminal) } finally { - closeSession(terminal.serverSessionId, terminal.serverListening) + closeSession(terminal.serverSessionId, terminal.serverListening, terminal = true) if (localSession.sessionId != null) { try { sessionRepository.updateLocalSession( @@ -2829,16 +2829,20 @@ class PlaybackManager @Inject constructor( private suspend fun syncTerminalSession(snapshot: TerminalPlaybackSnapshot) { val posSec = snapshot.position.toDouble(kotlin.time.DurationUnit.SECONDS) val durSec = snapshot.duration.toDouble(kotlin.time.DurationUnit.SECONDS) - if (snapshot.serverSessionId != null) { + if (snapshot.serverSessionId != null && terminalServerLive()) { try { snapshot.serverListening?.deliver(snapshot.timeListened) { delta -> - progressRepository.syncSessionProgress( - itemId = snapshot.bookId, - sessionId = snapshot.serverSessionId, - currentTime = posSec, - duration = durSec, - timeListened = delta, - ) + // Bounded inside deliver, whose own NonCancellable would + // otherwise outlast any timeout placed around it. + terminalServerCall(serverLive = true) { + progressRepository.syncSessionProgress( + itemId = snapshot.bookId, + sessionId = snapshot.serverSessionId, + currentTime = posSec, + duration = durSec, + timeListened = delta, + ) + } ?: false } } catch (cancellation: CancellationException) { throw cancellation @@ -2928,11 +2932,27 @@ class PlaybackManager @Inject constructor( } } - private suspend fun closeSession(sessionId: String?, listening: ServerSessionListening? = null) { + private fun terminalServerLive(): Boolean = terminalProgressPushAllowed( + isOnline = connectivityMonitor.isOnline.value, + connectionStatus = connectivityMonitor.connectionStatus.value, + ) + + private suspend fun closeSession( + sessionId: String?, + listening: ServerSessionListening? = null, + terminal: Boolean = false, + ) { if (sessionId == null) return - try { - if (listening != null) listening.close { apiService.closeSession(sessionId) } + // A stopped book's close holds up the next load, so it gets the terminal + // policy. A skipped or timed out close leaves the server session open + // until the server expires it; local state is cleared below either way. + val closeRemote: suspend () -> Unit = { + if (terminal) terminalServerCall(terminalServerLive()) { apiService.closeSession(sessionId) } else apiService.closeSession(sessionId) + } + try { + if (listening != null) listening.close(closeRemote) + else closeRemote() } catch (cancellation: CancellationException) { throw cancellation } catch (_: Exception) {} diff --git a/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt b/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt index 0b94c18f..01a884ea 100644 --- a/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/service/SyncManager.kt @@ -591,6 +591,19 @@ internal fun terminalProgressPushAllowed( (connectionStatus == ConnectivityMonitor.ConnectionStatus.CONNECTED || connectionStatus == ConnectivityMonitor.ConnectionStatus.SYNCING) +/** + * Run one of a stopped book's final server calls (session sync, session + * close) the way its progress push runs: not at all unless [serverLive], and + * given up after [timeout]. The next book's load waits on these. Returns null + * when skipped or timed out. The call must be cancellable, so bound it inside + * any NonCancellable wrapper, not outside. + */ +internal suspend fun terminalServerCall( + serverLive: Boolean, + timeout: kotlin.time.Duration = TERMINAL_PROGRESS_PUSH_TIMEOUT, + call: suspend () -> T, +): T? = if (serverLive) withTimeoutOrNull(timeout) { call() } else null + /** * Gate for a server sync. Requires an authenticated, non-LOCAL session AND an * active network. The online check is the "internet connection check" that diff --git a/app/src/test/java/com/ninelivesaudio/app/service/TerminalSessionCallTest.kt b/app/src/test/java/com/ninelivesaudio/app/service/TerminalSessionCallTest.kt new file mode 100644 index 00000000..1547a76f --- /dev/null +++ b/app/src/test/java/com/ninelivesaudio/app/service/TerminalSessionCallTest.kt @@ -0,0 +1,54 @@ +package com.ninelivesaudio.app.service + +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withContext +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import kotlin.time.Duration.Companion.milliseconds + +/** + * After a stopped book's final progress lands, the same terminal job sends + * the session's last sync and closes it on the server. The next book's load + * waits on that job, so with the server gone mid-listen those two calls held + * it for the 30s connect timeout each. They now follow the progress push: + * skipped unless the server is live, and given up at the same bound. + */ +class TerminalSessionCallTest { + + @Test + fun `a server that is not live gets no call`() = runBlocking { + var called = false + val result = terminalServerCall(serverLive = false) { called = true; true } + assertFalse(called) + assertNull(result) + } + + @Test(timeout = 5_000) + fun `a call that never answers gives up at the bound inside the terminal job`() = runBlocking { + val never = CompletableDeferred() + val started = System.nanoTime() + // The terminal job runs under NonCancellable. The bound must still hold. + val result = withContext(NonCancellable) { + terminalServerCall(serverLive = true, timeout = 100.milliseconds) { never.await() } + } + val elapsedMs = (System.nanoTime() - started) / 1_000_000 + assertNull(result) + assertTrue("waited ${elapsedMs}ms", elapsedMs < 2_000) + } + + @Test + fun `a live server that answers returns its result`() = runBlocking { + assertEquals(true, terminalServerCall(serverLive = true) { true }) + } + + @Test + fun `the default bound is the progress push bound`() = runBlocking { + // Guards against the two terminal bounds drifting apart. + assertEquals(5_000L, TERMINAL_PROGRESS_PUSH_TIMEOUT.inWholeMilliseconds) + } +} From d2683f0a4bbd79d9284961969efc0020ac477cbd Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 12:13:26 -0400 Subject: [PATCH 13/16] A background library reload can't strand the refresh spinner When a reconnect sync found a changed library list, its reload took the library lane and cancelled any manual refresh in flight. The cancelled refresh skipped clearing its spinner, and the replacement never cleared it either, so pull to refresh kept spinning over a loaded shelf. The reload now owns the same cleanup as refresh. Co-Authored-By: Claude Opus 5.5 --- .../app/ui/library/LibraryViewModel.kt | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt index 09abca97..42666b85 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt @@ -297,7 +297,10 @@ class LibraryViewModel @Inject constructor( cachedLibraryIds = cached.map { it.id }, ) ) { - libraryLoadLaunch.launch(viewModelScope) { loadLibraries() } + // This can replace a manual refresh in the lane, and the + // cancelled refresh never clears its spinner, so this + // load owns that cleanup. + libraryLoadLaunch.launch(viewModelScope) { loadLibrariesOwningRefresh() } } else { applyFilter() } @@ -580,12 +583,17 @@ class LibraryViewModel @Inject constructor( fun refresh() { libraryLoadLaunch.launch(viewModelScope) { _uiState.update { it.copy(isRefreshing = true) } - try { - loadLibraries() - } finally { - updateLibraryLoadStateIfActive { - _uiState.update { it.copy(isRefreshing = false) } - } + loadLibrariesOwningRefresh() + } + } + + /** Load, then clear the refresh spinner unless a newer load took over the lane. */ + private suspend fun loadLibrariesOwningRefresh() { + try { + loadLibraries() + } finally { + updateLibraryLoadStateIfActive { + _uiState.update { it.copy(isRefreshing = false) } } } } From 4f1147d9da67b1296a50bbc1e2c64efaef6a0743 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 13:49:39 -0400 Subject: [PATCH 14/16] A cold start behind a dead server waits on one reachability probe, not two loadLibraries probes /ping before fetching the library list, then its nested item load probed again. With the server gone behind a live VPN, the second 5s probe ran only after the first had already failed, so the saved shelf waited about 10s instead of 5. The item load now reuses the list's probe result when it has one. Co-Authored-By: Claude Opus 5.5 --- .../ninelivesaudio/app/ui/library/LibraryViewModel.kt | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt index 42666b85..1c80c9cf 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt @@ -324,13 +324,16 @@ class LibraryViewModel @Inject constructor( val serverUrlAtStart = settings.serverUrl val isLocalMode = settings.appMode == AppMode.LOCAL var libraryResult: RemoteResult>? = null + // The item load below reuses this probe so a dead server costs one + // 5s wait on a cold start, not two back to back. + var serverReachable: Boolean? = null val libs = if (isLocalMode) { libraryRepository.getLocalLibraries() } else { if (shouldSyncOnLibraryLoad( isLocalLibrary = false, isOnline = connectivityMonitor.isOnline.value, - ) && connectivityMonitor.checkServerReachable() + ) && connectivityMonitor.checkServerReachable().also { serverReachable = it } ) { refreshRemoteLibraryList( readCached = { @@ -366,7 +369,7 @@ class LibraryViewModel @Inject constructor( filterPublication.invalidate() val itemResult = selected?.let { - loadAudioBooks(it.id, persistResult = false) + loadAudioBooks(it.id, persistResult = false, serverReachable = serverReachable) } if (!isLocalMode) { buildShelfSyncReport(libraryResult, selected, itemResult)?.let { report -> @@ -395,6 +398,7 @@ class LibraryViewModel @Inject constructor( private suspend fun loadAudioBooks( libraryId: String, persistResult: Boolean = true, + serverReachable: Boolean? = null, ): RemoteResult>? { var itemResult: RemoteResult>? = null try { @@ -409,7 +413,7 @@ class LibraryViewModel @Inject constructor( if (shouldSyncOnLibraryLoad( isLocalLibrary = selected?.isLocal == true, isOnline = connectivityMonitor.isOnline.value, - ) && connectivityMonitor.checkServerReachable() + ) && (serverReachable ?: connectivityMonitor.checkServerReachable()) ) { itemResult = refreshSelectedLibraryItems( libraryId = libraryId, From e872aeeec8c58356db4700e898fa98a272270c42 Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 14:00:43 -0400 Subject: [PATCH 15/16] A library list shown in a different order no longer reloads forever After a sync, the Library compared its shown library IDs with the cached ones as ordered lists. A removed server library kept for its downloads is shown after the fetched ones, while the cache sorts by display order, so the same libraries read as a change. Every reload wrote a sync record that triggered the next reload. Membership is now compared as a set. Co-Authored-By: Claude Opus 5.5 --- .../com/ninelivesaudio/app/ui/library/LibraryViewModel.kt | 5 ++++- .../app/ui/library/LibraryReconnectReloadTest.kt | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt index 1c80c9cf..c2564a07 100644 --- a/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt +++ b/app/src/main/java/com/ninelivesaudio/app/ui/library/LibraryViewModel.kt @@ -105,13 +105,16 @@ internal fun shouldRequeryShelfAfterSync( * is a no-op, so an offline cold start with no saved libraries stayed empty * after the server returned. An empty cache with nothing shown stays put, or a * server with no libraries would reload on every sync record it writes. + * Membership is compared as a set: the shown list puts retained libraries + * after fetched ones while the cache sorts by display order, and treating + * that order difference as a change reloaded on every sync, forever. */ internal fun shouldReloadLibrariesAfterSync( selectedLibrary: Library?, shownLibraryIds: List, cachedLibraryIds: List, ): Boolean = (selectedLibrary == null && cachedLibraryIds.isNotEmpty()) || - cachedLibraryIds != shownLibraryIds + cachedLibraryIds.toSet() != shownLibraryIds.toSet() internal fun decideDownloadedOnlyFilter( previousStatus: ConnectionStatus, diff --git a/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryReconnectReloadTest.kt b/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryReconnectReloadTest.kt index 8566f46c..9ca85d48 100644 --- a/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryReconnectReloadTest.kt +++ b/app/src/test/java/com/ninelivesaudio/app/ui/library/LibraryReconnectReloadTest.kt @@ -37,4 +37,12 @@ class LibraryReconnectReloadTest { fun `a server with no libraries does not reload forever`() { assertFalse(shouldReloadLibrariesAfterSync(null, emptyList(), emptyList())) } + + @Test + fun `the same libraries in a different order only refilters`() { + // Retained libraries are shown after fetched ones while the cache sorts + // by display order. An order-only difference must not reload, or every + // reload's own sync record triggers the next one. + assertFalse(shouldReloadLibrariesAfterSync(books, listOf("books", "podcasts"), listOf("podcasts", "books"))) + } } From df5a2eaaa8f5d18d3a477dd7fcc06dd238dd749c Mon Sep 17 00:00:00 2001 From: Static Date: Wed, 30 Sep 2026 14:11:12 -0400 Subject: [PATCH 16/16] Self-signed trust applies to the configured server only With the self-signed toggle on, every HTTPS host went through the configured server's trust-on-first-use fingerprint and host check. An absolute playback URL or redirect to another host (a CDN or proxy) was refused even with a valid certificate. TOFU now applies only when the peer host is the configured server. Every other host gets the platform trust manager and hostname verifier, so a self-signed certificate anywhere else is still refused. Co-Authored-By: Claude Opus 5.5 --- .../data/remote/SelfSignedCertTrustManager.kt | 31 ++++++++++---- .../data/remote/SelfSignedCertTrustTest.kt | 42 +++++++++++++++++-- 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustManager.kt b/app/src/main/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustManager.kt index 4904cfec..b0a0441c 100644 --- a/app/src/main/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustManager.kt +++ b/app/src/main/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustManager.kt @@ -15,6 +15,7 @@ import javax.net.ssl.HostnameVerifier import javax.net.ssl.SSLContext import javax.net.ssl.SSLEngine import javax.net.ssl.SSLSession +import javax.net.ssl.SSLSocket import javax.net.ssl.TrustManager import javax.net.ssl.TrustManagerFactory import javax.net.ssl.X509ExtendedTrustManager @@ -76,12 +77,16 @@ object SelfSignedCertTrustManager { saveFingerprint = settingsManager::saveTrustedCertificateFingerprint, ) - val trustManager = PrefGatedTrustManager(platformTrustManager(), allowSelfSigned, tofu) + val isConfiguredHost = { host: String -> + val configured = runCatching { URI(settingsManager.currentSettings.serverUrl).host }.getOrNull() + configured != null && host.equals(configured, ignoreCase = true) + } + val trustManager = PrefGatedTrustManager(platformTrustManager(), allowSelfSigned, tofu, isConfiguredHost) val sslContext = SSLContext.getInstance("TLS") sslContext.init(null, arrayOf(trustManager), SecureRandom()) sslSocketFactory(sslContext.socketFactory, trustManager) - hostnameVerifier(PrefGatedHostnameVerifier(OkHostnameVerifier, allowSelfSigned, tofu)) + hostnameVerifier(PrefGatedHostnameVerifier(OkHostnameVerifier, allowSelfSigned, tofu, isConfiguredHost)) return this } @@ -97,14 +102,24 @@ object SelfSignedCertTrustManager { /** * Off: the platform trust manager, every overload forwarded as is. The socket * and engine forms matter on Android, where the Network Security Config trust - * manager needs the peer host they carry. On: trust-on-first-use. + * manager needs the peer host they carry. On: trust-on-first-use, but only for + * the configured server. Any other host (a CDN or proxy an absolute playback + * URL points at) keeps the platform check, since the pinned fingerprint is the + * configured server's and would reject a valid certificate elsewhere. */ internal class PrefGatedTrustManager( private val platform: X509TrustManager, private val allowSelfSigned: () -> Boolean, private val tofu: X509TrustManager, + private val isConfiguredHost: (String) -> Boolean = { true }, ) : X509ExtendedTrustManager() { + // The plain overloads carry no peer host, so they keep the configured + // server's TOFU behavior. Android's TLS stack calls the socket and engine + // forms, which do carry it. + private fun tofuFor(peerHost: String?): Boolean = + allowSelfSigned() && (peerHost == null || isConfiguredHost(peerHost)) + override fun checkClientTrusted(chain: Array?, authType: String?) { if (allowSelfSigned()) tofu.checkClientTrusted(chain, authType) else platform.checkClientTrusted(chain, authType) @@ -128,13 +143,13 @@ internal class PrefGatedTrustManager( } override fun checkServerTrusted(chain: Array?, authType: String?, socket: Socket?) { - if (allowSelfSigned()) tofu.checkServerTrusted(chain, authType) + if (tofuFor((socket as? SSLSocket)?.handshakeSession?.peerHost)) tofu.checkServerTrusted(chain, authType) else if (platform is X509ExtendedTrustManager) platform.checkServerTrusted(chain, authType, socket) else platform.checkServerTrusted(chain, authType) } override fun checkServerTrusted(chain: Array?, authType: String?, engine: SSLEngine?) { - if (allowSelfSigned()) tofu.checkServerTrusted(chain, authType) + if (tofuFor(engine?.peerHost)) tofu.checkServerTrusted(chain, authType) else if (platform is X509ExtendedTrustManager) platform.checkServerTrusted(chain, authType, engine) else platform.checkServerTrusted(chain, authType) } @@ -143,14 +158,16 @@ internal class PrefGatedTrustManager( if (allowSelfSigned()) tofu.acceptedIssuers else platform.acceptedIssuers } -/** Off: the platform verifier's answer. On: the configured host only, plus TOFU enrollment. */ +/** Off, or any host but the configured server: the platform verifier. On for the server: TOFU enrollment. */ internal class PrefGatedHostnameVerifier( private val platform: HostnameVerifier, private val allowSelfSigned: () -> Boolean, private val tofu: HostnameVerifier, + private val isConfiguredHost: (String) -> Boolean = { true }, ) : HostnameVerifier { override fun verify(hostname: String, session: SSLSession): Boolean = - if (allowSelfSigned()) tofu.verify(hostname, session) else platform.verify(hostname, session) + if (allowSelfSigned() && isConfiguredHost(hostname)) tofu.verify(hostname, session) + else platform.verify(hostname, session) } /** diff --git a/app/src/test/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustTest.kt b/app/src/test/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustTest.kt index 4bd4608d..99c55898 100644 --- a/app/src/test/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustTest.kt +++ b/app/src/test/java/com/ninelivesaudio/app/data/remote/SelfSignedCertTrustTest.kt @@ -34,11 +34,13 @@ class SelfSignedCertTrustTest { log = { _, _ -> }, ) + private val isConfiguredHost = { host: String -> host.equals("abs.local", ignoreCase = true) } + private fun gatedTrust(platform: X509TrustManager = SelfSignedCertTrustManager.platformTrustManager()) = - PrefGatedTrustManager(platform, { allow }, tofu) + PrefGatedTrustManager(platform, { allow }, tofu, isConfiguredHost) private fun gatedVerifier(platform: HostnameVerifier) = - PrefGatedHostnameVerifier(platform, { allow }, tofu) + PrefGatedHostnameVerifier(platform, { allow }, tofu, isConfiguredHost) // ─── Pref off: the platform decides, nothing else ───────────────────── @@ -94,9 +96,11 @@ class SelfSignedCertTrustTest { } @Test - fun `on refuses a host other than the configured server`() { + fun `on refuses another host whose certificate the platform rejects`() { + // Other hosts get the platform's answer, not TOFU, so a self-signed + // certificate on any host but the configured server stays refused. allow = true - val verifier = gatedVerifier { _, _ -> true } + val verifier = gatedVerifier { _, _ -> false } assertFalse(verifier.verify("evil.example", session(certA))) assertTrue(fingerprints.isEmpty()) } @@ -117,6 +121,36 @@ class SelfSignedCertTrustTest { // ─── Fixtures ───────────────────────────────────────────────────────── + // ─── Pref on, but another host: the platform decides ────────────────── + + @Test + fun `on hands a different host's hostname check to the platform`() { + allow = true + var consulted = 0 + val platform = HostnameVerifier { _, _ -> consulted++; true } + assertTrue(gatedVerifier(platform).verify("cdn.example.com", session(certB))) + assertEquals(1, consulted) + assertTrue("another host must never enroll", fingerprints.isEmpty()) + } + + @Test + fun `on hands a different host's chain to the platform trust manager`() { + allow = true + val platform = RecordingTrustManager() + val engine = javax.net.ssl.SSLContext.getDefault().createSSLEngine("cdn.example.com", 443) + gatedTrust(platform).checkServerTrusted(arrayOf(certB), "RSA", engine) + assertEquals(1, platform.serverChecks) + } + + @Test + fun `on keeps trust on first use for the configured server's engine`() { + allow = true + val platform = RecordingTrustManager() + val engine = javax.net.ssl.SSLContext.getDefault().createSSLEngine("abs.local", 13378) + gatedTrust(platform).checkServerTrusted(arrayOf(certA), "RSA", engine) + assertEquals("configured server must not reach the platform while on", 0, platform.serverChecks) + } + private class RecordingTrustManager : X509TrustManager { var serverChecks = 0 override fun checkClientTrusted(chain: Array?, authType: String?) = Unit