From 7b06b23860e5a01c7f76095c541ba84d6c32f5a1 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Fri, 25 Sep 2026 11:01:18 +0000 Subject: [PATCH] confirm the end of a full last page before failing the page cap A full page at the cap is not proof of truncation, so a library whose size is an exact multiple of the page size failed for no reason. Each paged loop now keeps going one page past the cap and only throws when that page comes back non-empty; a library up to the cap syncs normally. Add a boundary test for the exact-cap case. Also note the accepted per-batch observer invalidation churn in the sync code and the architecture doc. --- .../data/source/subsonic/SubsonicSource.kt | 28 +++++++++---------- .../com/subtracks/data/sync/SyncService.kt | 2 ++ .../source/subsonic/SubsonicSourceTest.kt | 21 ++++++++++++++ docs/architecture.md | 2 +- 4 files changed, 37 insertions(+), 16 deletions(-) diff --git a/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt b/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt index 8278d6d2..178d2628 100644 --- a/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt +++ b/app/src/main/kotlin/com/subtracks/data/source/subsonic/SubsonicSource.kt @@ -46,7 +46,7 @@ class SubsonicSource( val (frequent, recent) = fetchRanks() var offset = 0 var pages = 0 - while (pages < maxPages) { + while (true) { var count = 0 entityBatches( method = "getAlbumList2", @@ -67,10 +67,10 @@ class SubsonicSource( ) } if (count < PAGE_SIZE) break - offset += PAGE_SIZE pages++ + if (pages > maxPages) throw pageCapExceeded() + offset += PAGE_SIZE } - checkPageCap(pages) }.flowOn(Dispatchers.IO).buffer(1) override fun songs(): Flow> = @@ -138,7 +138,7 @@ class SubsonicSource( flow { var offset = 0 var pages = 0 - while (pages < maxPages) { + while (true) { var count = 0 entityBatches( method = "search3", @@ -158,17 +158,17 @@ class SubsonicSource( emit(batch) } if (count < PAGE_SIZE) break - offset += PAGE_SIZE pages++ + if (pages > maxPages) throw pageCapExceeded() + offset += PAGE_SIZE } - checkPageCap(pages) } private fun albumSongs(): Flow> = flow { var offset = 0 var pages = 0 - while (pages < maxPages) { + while (true) { val albums = ArrayList(PAGE_SIZE) entityBatches( method = "getAlbumList2", @@ -187,10 +187,10 @@ class SubsonicSource( ).collect { emit(it) } } if (albums.size < PAGE_SIZE) break - offset += PAGE_SIZE pages++ + if (pages > maxPages) throw pageCapExceeded() + offset += PAGE_SIZE } - checkPageCap(pages) } private suspend fun fetchRanks(): Pair, Map> = fetchRank("frequent") to fetchRank("recent") @@ -200,7 +200,7 @@ class SubsonicSource( val ranks = HashMap() var offset = 0 var pages = 0 - while (pages < maxPages) { + while (true) { var index = 0 entityBatches( method = "getAlbumList2", @@ -215,10 +215,10 @@ class SubsonicSource( } } if (index < PAGE_SIZE) break - offset += PAGE_SIZE pages++ + if (pages > maxPages) throw pageCapExceeded() + offset += PAGE_SIZE } - checkPageCap(pages) ranks } catch (_: SubsonicException) { emptyMap() @@ -239,9 +239,7 @@ class SubsonicSource( return supported } - private fun checkPageCap(pages: Int) { - if (pages >= maxPages) throw SubsonicException(-1, "Library exceeds the $maxPages page cap") - } + private fun pageCapExceeded() = SubsonicException(-1, "Library exceeds the ${maxPages * PAGE_SIZE}-row page cap") private fun entityBatches( method: String, diff --git a/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt b/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt index 9a91a425..ca137f4d 100644 --- a/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt +++ b/app/src/main/kotlin/com/subtracks/data/sync/SyncService.kt @@ -90,6 +90,8 @@ class SyncService( library.deleteOrphanPlaylistSongs(source.id) } + // ponytail: one commit per batch means Room notifies paged observers once per batch; gate + // UI observation on SyncStatus.Running if that churn is ever noticeable. private suspend fun write( table: String, columns: List, diff --git a/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt b/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt index a78d08c2..04cf5749 100644 --- a/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/source/subsonic/SubsonicSourceTest.kt @@ -203,6 +203,27 @@ class SubsonicSourceTest { assertEquals(true, failure.exceptionOrNull() is SubsonicException) } + @Test + fun albumsAtThePageCapSucceedWhenTheNextPageIsEmpty() = + runBlocking { + server.dispatcher = + object : Dispatcher() { + override fun dispatch(request: RecordedRequest): MockResponse { + val url = request.requestUrl!! + if (url.encodedPath != "/rest/getAlbumList2.view") return MockResponse().setResponseCode(404) + if (url.queryParameter("type") != "newest") return MockResponse().setBody(emptyAlbumList()) + return when (url.queryParameter("offset")) { + "0", "500" -> MockResponse().setBody(albumPage(url.queryParameter("offset")!!.toInt(), 500)) + else -> MockResponse().setBody(emptyAlbumList()) + } + } + } + + val albums = SubsonicSource(1, client(), maxPages = 2).albums().toList().flatten() + + assertEquals(1000, albums.size) + } + @Test fun playlistEntriesWithoutIdsAreSkipped() = runBlocking { diff --git a/docs/architecture.md b/docs/architecture.md index 50aa2f17..f7c39c7b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -112,7 +112,7 @@ Songs have a primary and a fallback fetch. The primary probes `search3` with an Cleanup is the "everything I had, minus everything the server just sent" step described above: artists, albums, songs and playlists delete the leftover IDs in 500-row chunks, disc titles are deleted per `(album, disc)` for albums that still exist, and playlist entries beyond the new count are removed along with entries for playlists that disappeared. -Sync is not atomic. Each batch commits on its own, so a failure leaves the batches already written in place and skips that entity's prune; stale rows can linger until the next successful sync, which also invalidates the queue's cached library. The one hazard is a scan that stops early — a server returning a short page by mistake, or a library larger than the page cap — because the prune trusts that the stream actually reached the end. +Sync is not atomic. Each batch commits on its own, so a failure leaves the batches already written in place and skips that entity's prune; stale rows can linger until the next successful sync, which also invalidates the queue's cached library. The one hazard is a scan that stops early — a server returning a short page by mistake, or a library larger than the page cap — because the prune trusts that the stream actually reached the end. Each batch commit also makes Room notify paged observers once per batch (a few per second, bounded by the network round-trips), which is cheap but can make an on-screen list churn during a sync; gate UI observation on `SyncStatus.Running` if that is ever noticeable. ## Search