diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/AuthCookieInterceptor.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/AuthCookieInterceptor.kt index e1a87926..172d767b 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/AuthCookieInterceptor.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/AuthCookieInterceptor.kt @@ -7,11 +7,23 @@ import javax.inject.Inject import javax.inject.Singleton /** - * Attaches the session cookie from [AuthStore] to every request, - * captures Set-Cookie from successful responses (login flow), and - * clears the store on 401 so downstream code can react to logout. + * Attaches the session cookie from [AuthStore] to Minstrel-server + * requests, captures Set-Cookie from successful responses (login + * flow), and clears the store on 401 so downstream code can react + * to logout. * * Mirrors the Flutter Dio interceptor pattern. + * + * Scoped to the [BaseUrlInterceptor.PLACEHOLDER_HOST] sentinel host + * the same way BaseUrlInterceptor is. Drift #568 / #569 caught two + * leaks here: (1) the session cookie was being attached to every + * external request the shared OkHttpClient services — including + * Coil image fetches to artwork.musicbrainz.org / coverartarchive.org + * / Lidarr's /MediaCover endpoints — exposing the session + * identifier to third-party logging; (2) a 401 from any of those + * external hosts silently wiped the user's Minstrel session. + * Restricting both attach + clear to placeholder-host requests + * closes both gaps. */ @Singleton class AuthCookieInterceptor @Inject constructor( @@ -19,8 +31,15 @@ class AuthCookieInterceptor @Inject constructor( ) : Interceptor { override fun intercept(chain: Interceptor.Chain): Response { + val original = chain.request() + if (original.url.host != BaseUrlInterceptor.PLACEHOLDER_HOST) { + // External request — no Minstrel session cookie attached, + // and a 401 from this host does NOT clear the user's + // session. Pass through untouched. + return chain.proceed(original) + } val cookie = authStore.sessionCookie.value - val request = chain.request().newBuilder().apply { + val request = original.newBuilder().apply { if (!cookie.isNullOrEmpty()) header("Cookie", cookie) }.build() diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedLikeDao.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedLikeDao.kt index 21b3d56c..a958ba15 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedLikeDao.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/db/dao/CachedLikeDao.kt @@ -4,6 +4,7 @@ import androidx.room.Dao import androidx.room.Insert import androidx.room.OnConflictStrategy import androidx.room.Query +import androidx.room.Transaction import com.fabledsword.minstrel.cache.db.entities.CachedLikeEntity import kotlinx.coroutines.flow.Flow @@ -36,4 +37,21 @@ interface CachedLikeDao { "WHERE userId = :userId AND entityType = :entityType AND entityId = :entityId", ) suspend fun delete(userId: String, entityType: String, entityId: String) + + @Query("DELETE FROM cached_likes WHERE userId = :userId") + suspend fun clearForUser(userId: String) + + /** + * Atomically replaces the user's entire cached_likes set with + * [rows]. Used by [com.fabledsword.minstrel.likes.data.LikesRepository.refreshIds] + * so cross-device unlikes (a row that the server no longer + * surfaces) get removed from the local cache — drift #570 + * caught the missing delete pass that left stale Liked tab + * entries pointing at tracks the user had unliked elsewhere. + */ + @Transaction + suspend fun replaceAllForUser(userId: String, rows: List) { + clearForUser(userId) + upsertAll(rows) + } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt index ae5a0ed9..63467dad 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/home/ui/HomeScreen.kt @@ -288,7 +288,15 @@ class HomeViewModel @Inject constructor( poolMessages.trySend("Mix isn't ready yet - try again in a moment") return@launch } - val source = if (playlist.refreshable) "playlist:${playlist.systemVariant}" else null + // Drift #564: send the BARE systemVariant string, not + // "playlist:" — the server's rotation matcher + // (internal/playevents/writer.go systemPlaylistSources) + // keys on the bare variant. Web sends the bare form too + // (web/src/lib/components/PlaylistCard.svelte:83), so this + // brings Android into alignment. Wrong prefix here meant + // system-mix plays from Android Home never advanced the + // rotation. + val source = if (playlist.refreshable) playlist.systemVariant else null player.setQueue(tracks, initialIndex = 0, source = source) }.join() } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/likes/data/LikesRepository.kt b/android/app/src/main/java/com/fabledsword/minstrel/likes/data/LikesRepository.kt index 1e5087ed..c876c7b6 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/likes/data/LikesRepository.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/likes/data/LikesRepository.kt @@ -1,19 +1,23 @@ package com.fabledsword.minstrel.likes.data import com.fabledsword.minstrel.api.endpoints.LikesApi +import com.fabledsword.minstrel.auth.AuthController import com.fabledsword.minstrel.cache.db.dao.CachedAlbumDao import com.fabledsword.minstrel.cache.db.dao.CachedArtistDao import com.fabledsword.minstrel.cache.db.dao.CachedLikeDao import com.fabledsword.minstrel.cache.db.dao.CachedTrackDao import com.fabledsword.minstrel.cache.db.entities.CachedLikeEntity import com.fabledsword.minstrel.cache.mutations.MutationQueue +import com.fabledsword.minstrel.di.ApplicationScope import com.fabledsword.minstrel.library.data.toDomain import com.fabledsword.minstrel.models.AlbumRef import com.fabledsword.minstrel.models.ArtistRef import com.fabledsword.minstrel.models.TrackRef +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map +import kotlinx.coroutines.launch import retrofit2.Retrofit import retrofit2.create import javax.inject.Inject @@ -24,11 +28,14 @@ import javax.inject.Singleton * Flutter `likesControllerProvider` + `cached_likes`-driven Liked tab * pattern. * - * Local `userId` discriminator is a constant for now (`LOCAL_USER_ID`) - * because the AuthController hookup lands with Phase 11. Single-user - * device is the only shape we target; if multi-tenant on one device - * ever shows up, swap this constant for `AuthStore.userId.value` - * everywhere and migrate the cached_likes rows. + * Local `userId` discriminator is the server-side user UUID + * (resolved via [AuthController.currentUser]); falls back to + * [ANONYMOUS_USER_ID] when no user is signed in so existing local + * cache rows from pre-#576 builds remain queryable until the first + * authenticated [refreshIds] call overwrites them. On user-switch + * (sign-out / sign-in as different user) the OUTGOING user's + * cached_likes rows are wiped so they don't leak into the new + * session — drift #576 audit caught the leak. * * Write path follows `feedback_offline_first_for_server_writes` — * never fire-and-forget: @@ -48,33 +55,64 @@ class LikesRepository @Inject constructor( private val artistDao: CachedArtistDao, private val trackDao: CachedTrackDao, private val mutationQueue: MutationQueue, + private val authController: AuthController, + @ApplicationScope private val scope: CoroutineScope, retrofit: Retrofit, ) { private val api: LikesApi = retrofit.create() + init { + // Drift #576: clear the outgoing user's cached_likes rows + // when the signed-in user changes. Mostly a hygiene fix — + // discriminator-based queries already prevent the wrong + // user from SEEING leaked rows, but without this the rows + // pile up forever as different accounts sign in/out on the + // same device. + scope.launch { + var previousId: String? = null + authController.currentUser.collect { user -> + val newId = user?.id + val prev = previousId + if (prev != null && prev != newId) { + likeDao.clearForUser(prev) + } + previousId = newId + } + } + } + + /** + * Server-user-uuid for the current session, or [ANONYMOUS_USER_ID] + * fallback when there's no signed-in user. Used as the + * `cached_likes.userId` discriminator so each account's likes are + * stored under its own bucket. + */ + private fun currentUserId(): String = + authController.currentUser.value?.id ?: ANONYMOUS_USER_ID + // ── Reads ───────────────────────────────────────────────────────── fun observeLikedArtists(): Flow> = - likeDao.observeLikedIdsOfType(LOCAL_USER_ID, ENTITY_ARTIST).map { ids -> + likeDao.observeLikedIdsOfType(currentUserId(), ENTITY_ARTIST).map { ids -> ids.mapNotNull { artistDao.getById(it)?.toDomain() } } fun observeLikedAlbums(): Flow> = - likeDao.observeLikedIdsOfType(LOCAL_USER_ID, ENTITY_ALBUM).map { ids -> + likeDao.observeLikedIdsOfType(currentUserId(), ENTITY_ALBUM).map { ids -> ids.mapNotNull { albumDao.getById(it)?.toDomain() } } fun observeLikedTracks(): Flow> = - likeDao.observeLikedIdsOfType(LOCAL_USER_ID, ENTITY_TRACK).map { ids -> + likeDao.observeLikedIdsOfType(currentUserId(), ENTITY_TRACK).map { ids -> ids.mapNotNull { trackDao.getById(it)?.toDomain() } } fun observeIsLiked(entityType: String, entityId: String): Flow = - likeDao.observeIsLiked(LOCAL_USER_ID, entityType, entityId) + likeDao.observeIsLiked(currentUserId(), entityType, entityId) /** One-shot snapshot of the liked track-id set — for the offline pool filter. */ suspend fun likedTrackIds(): Set = - likeDao.observeLikedIdsOfType(LOCAL_USER_ID, ENTITY_TRACK).first().toSet() + likeDao.observeLikedIdsOfType(currentUserId(), ENTITY_TRACK).first().toSet() // ── Writes (optimistic local → best-effort REST → enqueue on fail) ── @@ -85,13 +123,15 @@ class LikesRepository @Inject constructor( * if it got enqueued for later replay. */ suspend fun toggleLike(entityType: String, entityId: String, desiredState: Boolean): Boolean { - // 1. Optimistic Room mutation. + // 1. Optimistic Room mutation — scoped to the signed-in user + // (or the legacy "local" fallback for pre-#576 rows). + val uid = currentUserId() if (desiredState) { likeDao.upsertAll( - listOf(CachedLikeEntity(LOCAL_USER_ID, entityType, entityId)), + listOf(CachedLikeEntity(uid, entityType, entityId)), ) } else { - likeDao.delete(LOCAL_USER_ID, entityType, entityId) + likeDao.delete(uid, entityType, entityId) } // 2. Best-effort REST call; 3. enqueue on failure. val kindPath = serverPathFor(entityType) @@ -111,24 +151,38 @@ class LikesRepository @Inject constructor( } /** - * Pulls `GET /api/likes/ids` and reconciles `cached_likes`. Called - * by the LikedTab ViewModel on init (and by the SyncController in - * Phase 12). Adds rows for IDs the server has but we don't; the - * delete-of-stale-rows pass lives in the SyncController where the - * full reconciliation runs. + * Pulls `GET /api/likes/ids` and atomically replaces the user's + * cached_likes set with the server's canonical view. Called by + * the LikedTab ViewModel on init (and by the SyncController in + * Phase 12). + * + * Drift #570 fix: previously this called `upsertAll` only, which + * added rows for IDs the server had but never removed rows the + * server no longer surfaced — so a cross-device unlike (user + * likes on web, then unlikes on web) left the Liked tab on + * Android showing the now-unliked entry forever. The atomic + * replaceAllForUser DAO method runs a transactional delete-then- + * insert so the local set is exactly what the server reports. */ suspend fun refreshIds() { + val uid = currentUserId() val wire = api.ids() val rows = mutableListOf() - rows += wire.artistIds.map { CachedLikeEntity(LOCAL_USER_ID, ENTITY_ARTIST, it) } - rows += wire.albumIds.map { CachedLikeEntity(LOCAL_USER_ID, ENTITY_ALBUM, it) } - rows += wire.trackIds.map { CachedLikeEntity(LOCAL_USER_ID, ENTITY_TRACK, it) } - likeDao.upsertAll(rows) + rows += wire.artistIds.map { CachedLikeEntity(uid, ENTITY_ARTIST, it) } + rows += wire.albumIds.map { CachedLikeEntity(uid, ENTITY_ALBUM, it) } + rows += wire.trackIds.map { CachedLikeEntity(uid, ENTITY_TRACK, it) } + likeDao.replaceAllForUser(uid, rows) } companion object { - // TODO(Phase 11): swap for AuthStore.userId.value once auth lands. - const val LOCAL_USER_ID: String = "local" + // Pre-auth fallback discriminator. Drift #576: when no user is + // signed in OR for cached_likes rows written by pre-#576 + // builds (which all tagged "local"), reads continue to query + // this bucket so the Liked tab isn't suddenly empty after the + // upgrade. The first authenticated refreshIds() call writes + // rows under the real user UUID; reads then transparently + // switch over via currentUserId(). + const val ANONYMOUS_USER_ID: String = "local" const val ENTITY_ARTIST: String = "artist" const val ENTITY_ALBUM: String = "album" const val ENTITY_TRACK: String = "track" diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt index 31dac4b2..102bcba1 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt @@ -72,10 +72,19 @@ class PlayerController @Inject constructor( * failure (decoder, transport, EOS), and once when the player * reaches STATE_READY with a duration of zero / TIME_UNSET — the * "track loaded but has no audio" case the user sees as the - * player sitting frozen on a track. Conflated channel so back- - * pressure can't stall the player loop. + * player sitting frozen on a track. + * + * Drift #561: this was originally Channel.CONFLATED, which silently + * dropped every emission except the latest each time the reporter + * loop wasn't actively reading. A network blip that failed 5 + * tracks back-to-back would surface only the last failure to the + * snackbar (the "Skipped 5 unplayable tracks" coalescing path was + * dead code) AND only POST one playback_errors row to the admin + * inbox instead of 5. Buffered so every burst event reaches the + * reporter; default capacity is 64 which is well above any real + * burst rate. */ - private val playbackErrorEventsChannel = Channel(Channel.CONFLATED) + private val playbackErrorEventsChannel = Channel(Channel.BUFFERED) val playbackErrorEvents: Flow = playbackErrorEventsChannel.receiveAsFlow() /** @@ -93,10 +102,30 @@ class PlayerController @Inject constructor( */ private var queueRefs: List = emptyList() + /** + * Completes when [mediaController] is non-null and the listener has + * been attached. Used by [awaitReady] so cold-boot callers like + * [ResumeController] can wait for the IPC bind before calling + * transport methods that would otherwise no-op silently. Drift + * #562 caught the race where a fast restore() landed before the + * MediaSessionService connection was up, silently dropping the + * restored queue. + */ + private val readyDeferred = kotlinx.coroutines.CompletableDeferred() + init { scope.launch { connectAndObserve() } } + /** + * Suspends until the MediaController binding to + * [MinstrelPlayerService] is up and a Player.Listener is attached, + * so a subsequent transport call (setQueue, startRadio, playNext, + * …) will actually reach the player rather than being silently + * swallowed by the `mediaController ?: return` guards. + */ + suspend fun awaitReady(): Unit = readyDeferred.await() + // ── Transport (no-op until the controller is connected) ────────────── fun play() { mediaController?.play() } @@ -135,18 +164,32 @@ class PlayerController @Inject constructor( } /** - * Replace the queue with [tracks] and start playing from [initialIndex]. + * Replace the queue with [tracks] starting at [initialIndex]. * [source] tags the queue with its origin (e.g. "for_you") so the * server-side rotation reporter can advance it; carried in * MediaItem extras. + * + * [autoplay] controls whether playback starts immediately. The + * default is true to preserve the "user pressed play on a tile" + * UX every existing caller relies on. Drift #560: cold-boot + * resume passes autoplay = false so the persisted queue is + * restored without auto-starting playback after the user has + * been away from the app — starting audio on cold launch was + * surprising for users who had paused mid-track before + * backgrounding. */ - fun setQueue(tracks: List, initialIndex: Int = 0, source: String? = null) { + fun setQueue( + tracks: List, + initialIndex: Int = 0, + source: String? = null, + autoplay: Boolean = true, + ) { val controller = mediaController ?: return queueRefs = tracks val items = tracks.map { it.toMediaItem(source) } controller.setMediaItems(items, initialIndex, /* startPositionMs = */ 0L) controller.prepare() - controller.play() + if (autoplay) controller.play() } /** @@ -273,6 +316,11 @@ class PlayerController @Inject constructor( return } mediaController = controller + // Drift #562: signal awaitReady() callers (ResumeController et al.) + // that the controller is bound. Listener is attached below in + // the same coroutine so by the time downstream code runs after + // awaitReady, the Player.Listener is wired too. + if (!readyDeferred.isCompleted) readyDeferred.complete(Unit) startPositionPolling(controller) controller.addListener( object : Player.Listener { diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/ResumeController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/ResumeController.kt index 1f666738..fddf8e46 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/ResumeController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/ResumeController.kt @@ -54,12 +54,22 @@ class ResumeController @Inject constructor( .getOrNull() ?.takeIf { it.tracks.isNotEmpty() } ?: return + // Drift #562: wait for the MediaController binding before + // calling setQueue, otherwise restore() racing the IPC + // handshake silently drops the persisted queue (PlayerController + // setQueue early-returns when mediaController is null). + playerController.awaitReady() playerController.setQueue( tracks = payload.tracks, initialIndex = payload.queueIndex .coerceAtLeast(0) .coerceAtMost(payload.tracks.lastIndex), source = payload.source, + // Drift #560: cold-boot resume must NOT auto-start + // playback. The user has been away from the app; starting + // audio on cold launch is surprising. They tap play to + // resume. Queue + position are restored; transport is idle. + autoplay = false, ) // PositionMs not seeked yet — Player connection may still be // establishing; the seek call would no-op. Acceptable to start diff --git a/android/app/src/main/java/com/fabledsword/minstrel/playlists/ui/PlaylistDetailScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/playlists/ui/PlaylistDetailScreen.kt index 99250266..db8c3529 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/playlists/ui/PlaylistDetailScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/playlists/ui/PlaylistDetailScreen.kt @@ -227,7 +227,16 @@ class PlaylistDetailViewModel @Inject constructor( val refs = tracks.toPlayableTrackRefs() if (refs.isEmpty()) return val startIndex = refs.indexOfFirst { it.id == startTrackId }.coerceAtLeast(0) - player.setQueue(refs, initialIndex = startIndex, source = "playlist:$playlistId") + // Drift #564: for refreshable system playlists, send the bare + // systemVariant string so the rotation matcher + // (systemPlaylistSources in playevents/writer.go) sees the + // play. User playlists keep the "playlist:" tag for + // attribution but it doesn't trigger rotation (intentional — + // rotation only applies to system mixes). + val playlist = (internal.value as? PlaylistDetailUiState.Success)?.detail?.playlist + val source = playlist?.systemVariant?.takeIf { playlist.refreshable } + ?: "playlist:$playlistId" + player.setQueue(refs, initialIndex = startIndex, source = source) } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/requests/ui/RequestsViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/requests/ui/RequestsViewModel.kt index 1b938506..7759a78e 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/requests/ui/RequestsViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/requests/ui/RequestsViewModel.kt @@ -5,6 +5,7 @@ import androidx.lifecycle.viewModelScope import com.fabledsword.minstrel.api.ErrorCopy import com.fabledsword.minstrel.events.EventsStream import com.fabledsword.minstrel.models.RequestRef +import com.fabledsword.minstrel.requests.data.CancelOutcome import com.fabledsword.minstrel.requests.data.RequestsRepository import com.fabledsword.minstrel.shared.UiState import dagger.hilt.android.lifecycle.HiltViewModel @@ -71,7 +72,7 @@ class RequestsViewModel @Inject constructor( } viewModelScope.launch { try { - val (_, updated) = repository.cancel(id) + val (outcome, updated) = repository.cancel(id) if (updated != null) { internal.update { state -> if (state !is UiState.Success) { @@ -87,12 +88,20 @@ class RequestsViewModel @Inject constructor( } } } - // Refresh fetches the canonical list whether the cancel - // went through live (Synced) or got enqueued (Queued). - // The Queued case shows the optimistic removal until the - // replayer drains; the next list fetch surfaces the - // server's canonical view. - refresh() + // Drift #577: only refetch on the Synced outcome. The + // Queued path is offline by definition — the optimistic + // removal at the top of cancel() is correct, and a + // refresh() call here would either (a) get the + // not-yet-delivered cancelled row back from the server + // and snap it into the list (confusing), or (b) fail + // with a transport error and flip the whole screen to + // UiState.Error, making the user think the cancel + // failed. The mutation queue will replay the cancel + // when connectivity returns; the next on-screen refresh + // (pull-to-refresh, navigation back) reconciles. + if (outcome == CancelOutcome.Synced) { + refresh() + } } catch ( @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable, ) { diff --git a/android/app/src/test/java/com/fabledsword/minstrel/api/AuthCookieInterceptorTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/api/AuthCookieInterceptorTest.kt index 61a3c582..039e8a8a 100644 --- a/android/app/src/test/java/com/fabledsword/minstrel/api/AuthCookieInterceptorTest.kt +++ b/android/app/src/test/java/com/fabledsword/minstrel/api/AuthCookieInterceptorTest.kt @@ -45,12 +45,26 @@ class AuthCookieInterceptorTest { } authStore = AuthStore(dao, TestScope(UnconfinedTestDispatcher())) + // BaseUrlInterceptor rewrites placeholder.invalid → mock server. + // AuthCookieInterceptor scopes its attach + clear behavior to + // the placeholder host so external Coil image fetches don't get + // the Minstrel session cookie attached and don't trigger a + // session-clear on 401. Tests issue requests to + // http://placeholder.invalid/... so the auth interceptor sees + // the in-scope host, and BaseUrlInterceptor (running AFTER, so + // the auth interceptor sees the original host) rewrites the URL + // before transport. + authStore.setBaseUrl(server.url("/").toString()) client = OkHttpClient.Builder() .addInterceptor(AuthCookieInterceptor(authStore)) + .addInterceptor(BaseUrlInterceptor(authStore)) .build() } + private fun placeholderUrl(path: String): String = + "http://${BaseUrlInterceptor.PLACEHOLDER_HOST}$path" + @AfterEach fun teardown() { server.shutdown() @@ -61,7 +75,7 @@ class AuthCookieInterceptorTest { authStore.setSessionCookie("session=abc123") server.enqueue(MockResponse().setResponseCode(200)) - client.newCall(Request.Builder().url(server.url("/api/test")).build()).execute() + client.newCall(Request.Builder().url(placeholderUrl("/api/test")).build()).execute() val recorded = server.takeRequest() assertEquals("session=abc123", recorded.getHeader("Cookie")) @@ -72,7 +86,7 @@ class AuthCookieInterceptorTest { authStore.setSessionCookie("session=expired") server.enqueue(MockResponse().setResponseCode(401)) - client.newCall(Request.Builder().url(server.url("/api/test")).build()).execute() + client.newCall(Request.Builder().url(placeholderUrl("/api/test")).build()).execute() assertNull(authStore.sessionCookie.value) } @@ -85,7 +99,7 @@ class AuthCookieInterceptorTest { .addHeader("Set-Cookie", "session=newvalue; Path=/; HttpOnly"), ) - client.newCall(Request.Builder().url(server.url("/api/login")).build()).execute() + client.newCall(Request.Builder().url(placeholderUrl("/api/login")).build()).execute() assertEquals("session=newvalue", authStore.sessionCookie.value) } @@ -95,8 +109,41 @@ class AuthCookieInterceptorTest { authStore.setSessionCookie("session=existing") server.enqueue(MockResponse().setResponseCode(200)) - client.newCall(Request.Builder().url(server.url("/api/anything")).build()).execute() + client.newCall(Request.Builder().url(placeholderUrl("/api/anything")).build()).execute() assertEquals("session=existing", authStore.sessionCookie.value) } + + // Drift #568 regression guard: requests to external hosts must NOT + // carry the Minstrel session cookie even when the auth store has + // one. The shared OkHttpClient is also used by Coil for image + // fetches to artwork.musicbrainz.org and the like; leaking the + // session cookie to those hosts is a posture violation. + @Test + fun `does NOT attach cookie to non-placeholder host`() { + authStore.setSessionCookie("session=secret") + server.enqueue(MockResponse().setResponseCode(200)) + + // Request goes directly to the mock server's host:port — NOT + // through the placeholder sentinel — so the auth interceptor + // should pass it through untouched. + client.newCall(Request.Builder().url(server.url("/external/image.jpg")).build()).execute() + + val recorded = server.takeRequest() + assertNull(recorded.getHeader("Cookie")) + } + + // Drift #569 regression guard: a 401 from an external host must + // NOT wipe the user's Minstrel session. A misbehaving CDN or a + // Lidarr that's 401-ing on /MediaCover URLs used to silently sign + // the user out. + @Test + fun `does NOT clear cookie on 401 from non-placeholder host`() { + authStore.setSessionCookie("session=keep") + server.enqueue(MockResponse().setResponseCode(401)) + + client.newCall(Request.Builder().url(server.url("/external/image.jpg")).build()).execute() + + assertEquals("session=keep", authStore.sessionCookie.value) + } } diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index 5b67a423..04cfe6ff 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -15,6 +15,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" "git.fabledsword.com/bvandeusen/minstrel/internal/db" "git.fabledsword.com/bvandeusen/minstrel/internal/eventbus" + "git.fabledsword.com/bvandeusen/minstrel/internal/gc" "git.fabledsword.com/bvandeusen/minstrel/internal/library" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrconfig" @@ -161,6 +162,16 @@ func run() error { similarityWorker := similarity.NewWorker(pool, listenbrainz.NewClient(), logger.With("component", "similarity")) go similarityWorker.Run(ctx) + // Start the GC worker. Runs every 1h and sweeps lifecycle tables + // that have no writer-side close path or retention policy: + // orphan play_events, stale play_sessions, expired + // scrobble_queue failures, stuck system_playlist_runs, expired + // password_resets. Each sweep is idempotent — a row that's + // already clean is a no-op. Addresses drift audit findings + // #565 #566 #567 #574 #575 (Scribe parent #552). + gcWorker := gc.NewWorker(pool, logger.With("component", "gc")) + go gcWorker.Run(ctx) + // Start the Lidarr reconciler worker. Per spec §M5a, polls pending Lidarr // import requests and reconciles them against the library. Short-circuits // to no-op when lidarr_config.enabled = false. diff --git a/internal/api/admin_coverage_test.go b/internal/api/admin_coverage_test.go index dd4e3c07..4fbd4e0c 100644 --- a/internal/api/admin_coverage_test.go +++ b/internal/api/admin_coverage_test.go @@ -69,15 +69,32 @@ func TestAdminLibraryCoverage_MixedRowsReturnCorrectBuckets(t *testing.T) { sourceNone := "none" sourceSidecar := "sidecar" sourceMbcaa := "mbcaa" + sourceEmbedded := "embedded" + sourceTheaudiodb := "theaudiodb" + sourceDeezer := "deezer" + sourceLastfm := "lastfm" mbid1 := "11111111-1111-1111-1111-111111111111" mbid2 := "22222222-2222-2222-2222-222222222222" mbid3 := "33333333-3333-3333-3333-333333333333" + mbid4 := "44444444-4444-4444-4444-444444444444" + mbid5 := "55555555-5555-5555-5555-555555555555" + mbid6 := "66666666-6666-6666-6666-666666666666" + mbid7 := "77777777-7777-7777-7777-777777777777" + // Cover every cover_art_source value the migrations allow so a future + // addition without updating the rollup query trips this test — + // regression guard for drift #557, the gap that hid #556 (deezer + + // lastfm omission introduced by migration 0020 but never wired into + // the rollup whitelist). rows := []seed{ - {title: "WithArtSidecar", source: &sourceSidecar, mbid: &mbid1}, // with_art - {title: "WithArtMbcaa", source: &sourceMbcaa, mbid: &mbid2}, // with_art - {title: "PendingHasMbid", source: nil, mbid: &mbid3}, // pending (eligible) - {title: "PendingNoMbid", source: nil, mbid: nil}, // pending + pending_no_mbid - {title: "Settled", source: &sourceNone, mbid: nil}, // settled (no mbid is fine here) + {title: "WithArtSidecar", source: &sourceSidecar, mbid: &mbid1}, // with_art + {title: "WithArtMbcaa", source: &sourceMbcaa, mbid: &mbid2}, // with_art + {title: "WithArtEmbedded", source: &sourceEmbedded, mbid: &mbid4}, // with_art + {title: "WithArtTheaudiodb", source: &sourceTheaudiodb, mbid: &mbid5}, // with_art + {title: "WithArtDeezer", source: &sourceDeezer, mbid: &mbid6}, // with_art + {title: "WithArtLastfm", source: &sourceLastfm, mbid: &mbid7}, // with_art + {title: "PendingHasMbid", source: nil, mbid: &mbid3}, // pending (eligible) + {title: "PendingNoMbid", source: nil, mbid: nil}, // pending + pending_no_mbid + {title: "Settled", source: &sourceNone, mbid: nil}, // settled (no mbid is fine here) } for _, s := range rows { if _, err := pool.Exec(context.Background(), ` @@ -100,11 +117,13 @@ func TestAdminLibraryCoverage_MixedRowsReturnCorrectBuckets(t *testing.T) { if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { t.Fatalf("decode: %v", err) } - if resp.Total != 5 { - t.Errorf("Total = %d, want 5", resp.Total) + if resp.Total != 9 { + t.Errorf("Total = %d, want 9", resp.Total) } - if resp.WithArt != 2 { - t.Errorf("WithArt = %d, want 2", resp.WithArt) + // Six with_art rows — one per valid cover_art_source value + // (sidecar, mbcaa, embedded, theaudiodb, deezer, lastfm). + if resp.WithArt != 6 { + t.Errorf("WithArt = %d, want 6", resp.WithArt) } if resp.Pending != 2 { t.Errorf("Pending = %d, want 2", resp.Pending) diff --git a/internal/api/me.go b/internal/api/me.go index 5372dfd1..bc33cd8f 100644 --- a/internal/api/me.go +++ b/internal/api/me.go @@ -1,7 +1,6 @@ package api import ( - "encoding/json" "errors" "net/http" @@ -9,6 +8,22 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/auth" ) +// handleGetMe returns the authenticated user's profile shape — id, +// username, display_name, email, is_admin. Mirrors the response shape +// of PUT /api/me/profile so the Android Settings → Profile screen +// (and Flutter's equivalent) can read its starting state from a GET +// before the user has done a first save. +// +// Drift #578 fix: this previously emitted the narrower UserView +// (id, username, is_admin only). Android's MeApi.getProfile() deser- +// ialised into MyProfileWire and saw display_name=null, email=null +// on every read — wiping the form fields on every Settings open. +// Saving from that blank state then submitted empty strings, which +// handleUpdateMyProfile treats as "clear to NULL" — DESTROYING the +// user's stored display_name + email. Returning the full profile +// shape here closes the loop. Web (User type narrower than this +// response) keeps working via TypeScript structural typing — the +// extra fields are ignored. func (h *handlers) handleGetMe(w http.ResponseWriter, r *http.Request) { user, ok := auth.UserFromContext(r.Context()) if !ok { @@ -18,10 +33,5 @@ func (h *handlers) handleGetMe(w http.ResponseWriter, r *http.Request) { writeErr(w, apierror.InternalMsg("missing auth context", errors.New("missing auth context"))) return } - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(UserView{ - ID: user.ID, - Username: user.Username, - IsAdmin: user.IsAdmin, - }) + writeJSON(w, http.StatusOK, profileViewFromUser(user)) } diff --git a/internal/api/me_test.go b/internal/api/me_test.go index 5f37b92e..a9515e6c 100644 --- a/internal/api/me_test.go +++ b/internal/api/me_test.go @@ -1,6 +1,7 @@ package api import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -21,13 +22,61 @@ func TestHandleGetMe_ReturnsAuthenticatedUser(t *testing.T) { if w.Code != http.StatusOK { t.Fatalf("status = %d body = %s", w.Code, w.Body.String()) } - var got UserView + var got meProfileResp if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { t.Fatalf("decode: %v", err) } if got.Username != "test-alice" || !got.IsAdmin { t.Errorf("user = %+v, want test-alice/admin", got) } + if got.DisplayName != nil { + t.Errorf("DisplayName = %v, want nil for a user with no profile set", *got.DisplayName) + } + if got.Email != nil { + t.Errorf("Email = %v, want nil for a user with no profile set", *got.Email) + } +} + +// Drift #578 regression guard: a user whose profile fields ARE set +// must see them in the /api/me response. Previously this handler +// emitted the narrower UserView so Android Settings → Profile saw +// display_name=null and email=null on every read, wiped the form +// fields, and submitting from that blank state destroyed the user's +// stored values via the "empty string clears to NULL" semantics of +// PUT /api/me/profile. +func TestHandleGetMe_IncludesDisplayNameAndEmail(t *testing.T) { + h, pool := testHandlers(t) + user := seedUser(t, pool, "alice", "hunter2", false) + + displayName := "Alice Liddell" + email := "alice@example.com" + updated, err := dbq.New(pool).UpdateUserProfile(context.Background(), dbq.UpdateUserProfileParams{ + ID: user.ID, + DisplayName: &displayName, + Email: &email, + }) + if err != nil { + t.Fatalf("UpdateUserProfile: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/api/me", nil) + req = withUser(req, updated) + w := httptest.NewRecorder() + h.handleGetMe(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d body = %s", w.Code, w.Body.String()) + } + var got meProfileResp + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if got.DisplayName == nil || *got.DisplayName != displayName { + t.Errorf("DisplayName = %v, want %q", got.DisplayName, displayName) + } + if got.Email == nil || *got.Email != email { + t.Errorf("Email = %v, want %q", got.Email, email) + } } func TestHandleGetMe_MissingContextReturns500(t *testing.T) { diff --git a/internal/db/dbq/albums.sql.go b/internal/db/dbq/albums.sql.go index ca990688..3ebea2a2 100644 --- a/internal/db/dbq/albums.sql.go +++ b/internal/db/dbq/albums.sql.go @@ -126,7 +126,8 @@ func (q *Queries) GetAlbumByID(ctx context.Context, id pgtype.UUID) (Album, erro const getAlbumCoverageRollup = `-- name: GetAlbumCoverageRollup :one SELECT COUNT(*) AS total, - COUNT(*) FILTER (WHERE cover_art_source IN ('sidecar','embedded','mbcaa','theaudiodb')) AS with_art, + COUNT(*) FILTER (WHERE cover_art_source IN + ('sidecar','embedded','mbcaa','theaudiodb','deezer','lastfm')) AS with_art, COUNT(*) FILTER (WHERE cover_art_source IS NULL) AS pending, COUNT(*) FILTER (WHERE cover_art_source = 'none') AS settled, COUNT(*) FILTER (WHERE cover_art_source IS NULL AND mbid IS NULL) AS pending_no_mbid @@ -151,10 +152,12 @@ type GetAlbumCoverageRollupRow struct { // Invariant: with_art + pending + settled = total. (pending_no_mbid is // not part of the sum — it's a subset of pending, surfaced separately.) // -// The IN list below ('sidecar','embedded','mbcaa','theaudiodb') must stay in sync -// with the cover_art_source CHECK constraint in migration -// 0016_album_cover_source.up.sql. If a new source value is added there -// without updating this query, with_art will silently undercount. +// The IN list below must stay in sync with the cover_art_source CHECK +// constraint — currently relaxed by migration 0020 to include 'deezer' +// and 'lastfm', and migration 0030 further relaxed it to any non-empty +// string. If a new source value is added without updating this query, +// with_art will silently undercount. Drift #556 caught the deezer + +// lastfm omission introduced by migration 0020. func (q *Queries) GetAlbumCoverageRollup(ctx context.Context) (GetAlbumCoverageRollupRow, error) { row := q.db.QueryRow(ctx, getAlbumCoverageRollup) var i GetAlbumCoverageRollupRow diff --git a/internal/db/dbq/gc.sql.go b/internal/db/dbq/gc.sql.go new file mode 100644 index 00000000..cb150262 --- /dev/null +++ b/internal/db/dbq/gc.sql.go @@ -0,0 +1,126 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: gc.sql + +package dbq + +import ( + "context" +) + +const gcClosePlaySessionsWithNoRecentEvents = `-- name: GcClosePlaySessionsWithNoRecentEvents :execrows +UPDATE play_sessions + SET ended_at = COALESCE(last_event_at, started_at) + WHERE ended_at IS NULL + AND ( + (track_count > 0 AND last_event_at < now() - INTERVAL '6 hours') + OR + (track_count = 0 AND started_at < now() - INTERVAL '1 hour') + ) +` + +// #565: play_sessions.ended_at was added but never populated by any +// writer. Close sessions whose last_event_at is older than 6h — +// treating that as "user moved on" the same way audio_service does +// after grace periods. Sessions with NO events (track_count = 0) +// older than 1h are also closed (stale handshakes from clients that +// never recorded a play). ended_at is set to last_event_at so the +// session's duration reads naturally. +func (q *Queries) GcClosePlaySessionsWithNoRecentEvents(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcClosePlaySessionsWithNoRecentEvents) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const gcCloseStalePlayEvents = `-- name: GcCloseStalePlayEvents :execrows + +UPDATE play_events + SET ended_at = COALESCE( + started_at + (duration_played_ms * INTERVAL '1 millisecond'), + now() + ) + WHERE ended_at IS NULL + AND started_at < now() - INTERVAL '24 hours' +` + +// Background garbage-collector / lifecycle queries. All five address +// drift findings from the 2026-06-02 audit (Scribe parent #552): +// #565 #566 #567 #574 #575. Sweeper runs on a long tick (default 1h) +// so per-query cost is amortised; each is idempotent (re-running on +// already-closed/-deleted rows is a no-op). +// #566: play_events rows opened more than 24h ago that never got a +// play_ended. Client crashed mid-track, network dropped, etc. We +// synthesize ended_at = started_at + duration_played_ms when present, +// otherwise leave duration_played_ms null and stamp ended_at = now() +// so the row stops looking "open" for downstream queries that filter +// ended_at IS NULL. Doesn't touch was_skipped (we genuinely don't +// know if the user skipped). +func (q *Queries) GcCloseStalePlayEvents(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcCloseStalePlayEvents) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const gcDeleteExpiredPasswordResets = `-- name: GcDeleteExpiredPasswordResets :execrows +DELETE FROM password_resets + WHERE (used_at IS NOT NULL AND used_at < now() - INTERVAL '7 days') + OR (used_at IS NULL AND expires_at < now() - INTERVAL '1 hour') +` + +// #575: password_resets accumulates expired + used rows forever. The +// validation path already rejects them; this just keeps the table +// from growing unbounded. Used rows are kept for 7 days for audit; +// unused expired rows go immediately. +func (q *Queries) GcDeleteExpiredPasswordResets(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteExpiredPasswordResets) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const gcExpireScrobbleQueueFailedRows = `-- name: GcExpireScrobbleQueueFailedRows :execrows +DELETE FROM scrobble_queue + WHERE status = 'failed' + AND enqueued_at < now() - INTERVAL '14 days' +` + +// #567: scrobble_queue rows that have been in status='failed' for +// more than 14 days. The worker stops retrying after maxAttempts; +// failed rows accumulate forever otherwise. CASCADE from play_events +// already drops the row when the underlying event is deleted, so this +// only handles persistent failures (token revoked, etc.). +func (q *Queries) GcExpireScrobbleQueueFailedRows(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcExpireScrobbleQueueFailedRows) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const gcResetStuckSystemPlaylistRuns = `-- name: GcResetStuckSystemPlaylistRuns :execrows +UPDATE system_playlist_runs + SET in_flight = false, + last_error = COALESCE(last_error, 'stuck-row auto-reset by gc') + WHERE in_flight = true + AND last_run_at < now() - INTERVAL '10 minutes' +` + +// #574: system_playlist_runs.in_flight = true can wedge on a +// goroutine panic between SET in_flight=true and SET in_flight=false. +// The duplicate-prevention check refuses to start a fresh regen while +// in_flight, so a stuck row blocks all future regens for that user. +// Reset rows where last_run_at is older than 10 minutes (regens +// shouldn't take that long). +func (q *Queries) GcResetStuckSystemPlaylistRuns(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcResetStuckSystemPlaylistRuns) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} diff --git a/internal/db/queries/albums.sql b/internal/db/queries/albums.sql index 9811a083..4aaaadd0 100644 --- a/internal/db/queries/albums.sql +++ b/internal/db/queries/albums.sql @@ -156,13 +156,16 @@ SELECT a.id AS album_id, -- Invariant: with_art + pending + settled = total. (pending_no_mbid is -- not part of the sum — it's a subset of pending, surfaced separately.) -- --- The IN list below ('sidecar','embedded','mbcaa','theaudiodb') must stay in sync --- with the cover_art_source CHECK constraint in migration --- 0016_album_cover_source.up.sql. If a new source value is added there --- without updating this query, with_art will silently undercount. +-- The IN list below must stay in sync with the cover_art_source CHECK +-- constraint — currently relaxed by migration 0020 to include 'deezer' +-- and 'lastfm', and migration 0030 further relaxed it to any non-empty +-- string. If a new source value is added without updating this query, +-- with_art will silently undercount. Drift #556 caught the deezer + +-- lastfm omission introduced by migration 0020. SELECT COUNT(*) AS total, - COUNT(*) FILTER (WHERE cover_art_source IN ('sidecar','embedded','mbcaa','theaudiodb')) AS with_art, + COUNT(*) FILTER (WHERE cover_art_source IN + ('sidecar','embedded','mbcaa','theaudiodb','deezer','lastfm')) AS with_art, COUNT(*) FILTER (WHERE cover_art_source IS NULL) AS pending, COUNT(*) FILTER (WHERE cover_art_source = 'none') AS settled, COUNT(*) FILTER (WHERE cover_art_source IS NULL AND mbid IS NULL) AS pending_no_mbid diff --git a/internal/db/queries/gc.sql b/internal/db/queries/gc.sql new file mode 100644 index 00000000..26c0c202 --- /dev/null +++ b/internal/db/queries/gc.sql @@ -0,0 +1,70 @@ +-- Background garbage-collector / lifecycle queries. All five address +-- drift findings from the 2026-06-02 audit (Scribe parent #552): +-- #565 #566 #567 #574 #575. Sweeper runs on a long tick (default 1h) +-- so per-query cost is amortised; each is idempotent (re-running on +-- already-closed/-deleted rows is a no-op). + +-- name: GcCloseStalePlayEvents :execrows +-- #566: play_events rows opened more than 24h ago that never got a +-- play_ended. Client crashed mid-track, network dropped, etc. We +-- synthesize ended_at = started_at + duration_played_ms when present, +-- otherwise leave duration_played_ms null and stamp ended_at = now() +-- so the row stops looking "open" for downstream queries that filter +-- ended_at IS NULL. Doesn't touch was_skipped (we genuinely don't +-- know if the user skipped). +UPDATE play_events + SET ended_at = COALESCE( + started_at + (duration_played_ms * INTERVAL '1 millisecond'), + now() + ) + WHERE ended_at IS NULL + AND started_at < now() - INTERVAL '24 hours'; + +-- name: GcClosePlaySessionsWithNoRecentEvents :execrows +-- #565: play_sessions.ended_at was added but never populated by any +-- writer. Close sessions whose last_event_at is older than 6h — +-- treating that as "user moved on" the same way audio_service does +-- after grace periods. Sessions with NO events (track_count = 0) +-- older than 1h are also closed (stale handshakes from clients that +-- never recorded a play). ended_at is set to last_event_at so the +-- session's duration reads naturally. +UPDATE play_sessions + SET ended_at = COALESCE(last_event_at, started_at) + WHERE ended_at IS NULL + AND ( + (track_count > 0 AND last_event_at < now() - INTERVAL '6 hours') + OR + (track_count = 0 AND started_at < now() - INTERVAL '1 hour') + ); + +-- name: GcExpireScrobbleQueueFailedRows :execrows +-- #567: scrobble_queue rows that have been in status='failed' for +-- more than 14 days. The worker stops retrying after maxAttempts; +-- failed rows accumulate forever otherwise. CASCADE from play_events +-- already drops the row when the underlying event is deleted, so this +-- only handles persistent failures (token revoked, etc.). +DELETE FROM scrobble_queue + WHERE status = 'failed' + AND enqueued_at < now() - INTERVAL '14 days'; + +-- name: GcResetStuckSystemPlaylistRuns :execrows +-- #574: system_playlist_runs.in_flight = true can wedge on a +-- goroutine panic between SET in_flight=true and SET in_flight=false. +-- The duplicate-prevention check refuses to start a fresh regen while +-- in_flight, so a stuck row blocks all future regens for that user. +-- Reset rows where last_run_at is older than 10 minutes (regens +-- shouldn't take that long). +UPDATE system_playlist_runs + SET in_flight = false, + last_error = COALESCE(last_error, 'stuck-row auto-reset by gc') + WHERE in_flight = true + AND last_run_at < now() - INTERVAL '10 minutes'; + +-- name: GcDeleteExpiredPasswordResets :execrows +-- #575: password_resets accumulates expired + used rows forever. The +-- validation path already rejects them; this just keeps the table +-- from growing unbounded. Used rows are kept for 7 days for audit; +-- unused expired rows go immediately. +DELETE FROM password_resets + WHERE (used_at IS NOT NULL AND used_at < now() - INTERVAL '7 days') + OR (used_at IS NULL AND expires_at < now() - INTERVAL '1 hour'); diff --git a/internal/gc/worker.go b/internal/gc/worker.go new file mode 100644 index 00000000..22b42269 --- /dev/null +++ b/internal/gc/worker.go @@ -0,0 +1,101 @@ +// Package gc runs periodic garbage-collection / lifecycle sweeps +// against tables that have NO writer-side close path or NO retention +// policy. Each sweep addresses a drift finding from the 2026-06-02 +// audit (Scribe parent #552) and is idempotent — re-running it on +// already-clean rows is a no-op. +// +// One Worker handles all sweeps so a single long-tick goroutine +// amortises the per-tick fixed cost. Each individual sweep is small +// (single UPDATE / DELETE with a time-bounded WHERE) and emits a +// log line with the affected-row count so the sweep cadence is +// visible in the application log without an explicit metrics layer. +// +// Sweeps: +// - GcCloseStalePlayEvents (#566) +// - GcClosePlaySessionsWithNoRecentEvents (#565) +// - GcExpireScrobbleQueueFailedRows (#567) +// - GcResetStuckSystemPlaylistRuns (#574) +// - GcDeleteExpiredPasswordResets (#575) +package gc + +import ( + "context" + "log/slog" + "time" + + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// defaultTick is the production sweep cadence. 1 hour is generous +// since each sweep's WHERE clause uses a multi-hour staleness +// threshold; the worst-case delay between a row becoming sweepable +// and the worker noticing is bounded by tick + threshold. +const defaultTick = 1 * time.Hour + +// Worker holds the pool + logger + tick interval. Construct with +// [NewWorker]; pass the returned Worker to a goroutine that calls +// [Worker.Run] with a context that's cancelled on shutdown. +type Worker struct { + pool *pgxpool.Pool + logger *slog.Logger + tick time.Duration +} + +// NewWorker builds a Worker with the production tick (1h). Tests can +// reach into the Worker after construction to override `tick` for +// faster iteration. +func NewWorker(pool *pgxpool.Pool, logger *slog.Logger) *Worker { + return &Worker{pool: pool, logger: logger, tick: defaultTick} +} + +// Run blocks until ctx is cancelled, running every sweep on each +// tick. Sweeps fire in fixed order; an error in one does NOT abort +// the rest (the panic-vs-just-failed distinction matters here — a +// pgx transient error from one query shouldn't prevent the others +// from running). +func (w *Worker) Run(ctx context.Context) { + // Fire once at start so a freshly-deployed server doesn't wait a + // full tick before doing the initial sweep. Matches the scrobble + // + similarity workers' "sweep then tick" pattern. + w.tickOnce(ctx) + t := time.NewTicker(w.tick) + defer t.Stop() + for { + select { + case <-ctx.Done(): + return + case <-t.C: + w.tickOnce(ctx) + } + } +} + +// tickOnce runs each sweep once, logging the affected-row count. +// Errors are logged per-sweep but do NOT abort the remaining ones — +// each sweep is independent. +func (w *Worker) tickOnce(ctx context.Context) { + q := dbq.New(w.pool) + w.runSweep(ctx, "close_stale_play_events", q.GcCloseStalePlayEvents) + w.runSweep(ctx, "close_play_sessions", q.GcClosePlaySessionsWithNoRecentEvents) + w.runSweep(ctx, "expire_scrobble_failed", q.GcExpireScrobbleQueueFailedRows) + w.runSweep(ctx, "reset_stuck_system_runs", q.GcResetStuckSystemPlaylistRuns) + w.runSweep(ctx, "delete_expired_password_resets", q.GcDeleteExpiredPasswordResets) +} + +// runSweep is a small adapter so each sweep call site is a one-liner +// in tickOnce. Logs at info on rows>0 and debug on rows=0 to keep +// the normal-case (nothing-to-do) noise out of operator logs. +func (w *Worker) runSweep(ctx context.Context, name string, fn func(context.Context) (int64, error)) { + rows, err := fn(ctx) + if err != nil { + w.logger.Error("gc sweep failed", "sweep", name, "err", err) + return + } + if rows > 0 { + w.logger.Info("gc sweep", "sweep", name, "rows_affected", rows) + } else { + w.logger.Debug("gc sweep", "sweep", name, "rows_affected", 0) + } +} diff --git a/internal/gc/worker_test.go b/internal/gc/worker_test.go new file mode 100644 index 00000000..9cfd24c9 --- /dev/null +++ b/internal/gc/worker_test.go @@ -0,0 +1,320 @@ +package gc + +import ( + "context" + "io" + "log/slog" + "os" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" +) + +// testWorker constructs a Worker against MINSTREL_TEST_DATABASE_URL. +// Mirrors the api package's testHandlers pattern — skip when not in +// integration mode, migrate + reset, return the pool for the caller +// to seed. +func testWorker(t *testing.T) (*Worker, *pgxpool.Pool) { + t.Helper() + if testing.Short() { + t.Skip("skipping gc integration in -short mode") + } + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + if err := db.Migrate(dsn, logger); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(context.Background(), dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + dbtest.ResetDB(t, pool) + return NewWorker(pool, logger), pool +} + +// seedUser creates a minimal user row for tests that need play_events +// / play_sessions / scrobble_queue rows. Username is prefixed so +// dbtest.ResetDB cleans up between runs. +func seedUser(t *testing.T, pool *pgxpool.Pool, name string) pgtype.UUID { + t.Helper() + u, err := dbq.New(pool).CreateUser(context.Background(), dbq.CreateUserParams{ + Username: dbtest.TestUserPrefix + name, + PasswordHash: "test-hash", + ApiToken: "test-token-" + name, + IsAdmin: false, + }) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + return u.ID +} + +func TestGcCloseStalePlayEvents_ClosesOnly24hOldRows(t *testing.T) { + w, pool := testWorker(t) + ctx := context.Background() + userID := seedUser(t, pool, "alice") + + // Need a track + session to satisfy FKs on play_events. + var trackID pgtype.UUID + var artistID pgtype.UUID + // `artists` has sort_name NOT NULL; mirror the title in sort_name + // like every real insert site does (see api.search / library scan). + if err := pool.QueryRow(ctx, ` + INSERT INTO artists (name, sort_name) VALUES ('A', 'A') RETURNING id + `).Scan(&artistID); err != nil { + t.Fatalf("seed artist row: %v", err) + } + var albumID pgtype.UUID + if err := pool.QueryRow(ctx, ` + INSERT INTO albums (artist_id, title, sort_title) VALUES ($1, 'X', 'X') RETURNING id + `, artistID).Scan(&albumID); err != nil { + t.Fatalf("seed album: %v", err) + } + // `tracks` has file_size + file_format NOT NULL. Use plausible + // stub values — the GC sweep doesn't read any of these columns, + // it only joins on track_id. + if err := pool.QueryRow(ctx, ` + INSERT INTO tracks (album_id, artist_id, title, file_path, + duration_ms, file_size, file_format) + VALUES ($1, $2, 'T', '/x.mp3', 180000, 4_000_000, 'mp3') + RETURNING id + `, albumID, artistID).Scan(&trackID); err != nil { + t.Fatalf("seed track: %v", err) + } + var sessionID pgtype.UUID + if err := pool.QueryRow(ctx, ` + INSERT INTO play_sessions (user_id, started_at, last_event_at) + VALUES ($1, now() - interval '2 hours', now()) RETURNING id + `, userID).Scan(&sessionID); err != nil { + t.Fatalf("seed session: %v", err) + } + + // Stale row (25h old, no ended_at) — should be closed. + if _, err := pool.Exec(ctx, ` + INSERT INTO play_events (user_id, track_id, session_id, started_at) + VALUES ($1, $2, $3, now() - interval '25 hours') + `, userID, trackID, sessionID); err != nil { + t.Fatalf("seed stale event: %v", err) + } + // Fresh row (1h old, no ended_at) — should be left alone. + if _, err := pool.Exec(ctx, ` + INSERT INTO play_events (user_id, track_id, session_id, started_at) + VALUES ($1, $2, $3, now() - interval '1 hour') + `, userID, trackID, sessionID); err != nil { + t.Fatalf("seed fresh event: %v", err) + } + + w.tickOnce(ctx) + + var closedStale, openFresh bool + if err := pool.QueryRow(ctx, ` + SELECT EXISTS(SELECT 1 FROM play_events + WHERE started_at < now() - interval '24 hours' + AND ended_at IS NOT NULL) + `).Scan(&closedStale); err != nil { + t.Fatalf("check stale: %v", err) + } + if err := pool.QueryRow(ctx, ` + SELECT EXISTS(SELECT 1 FROM play_events + WHERE started_at > now() - interval '2 hours' + AND ended_at IS NULL) + `).Scan(&openFresh); err != nil { + t.Fatalf("check fresh: %v", err) + } + if !closedStale { + t.Errorf("stale play_events row not closed") + } + if !openFresh { + t.Errorf("fresh play_events row was closed (should be left alone)") + } +} + +func TestGcClosePlaySessions_ClosesIdleAndEmptyStarvedSessions(t *testing.T) { + w, pool := testWorker(t) + ctx := context.Background() + userID := seedUser(t, pool, "bob") + + // Idle session — has events, last_event_at 8h ago. + if _, err := pool.Exec(ctx, ` + INSERT INTO play_sessions (user_id, started_at, last_event_at, track_count) + VALUES ($1, now() - interval '8 hours', now() - interval '8 hours', 5) + `, userID); err != nil { + t.Fatalf("seed idle session: %v", err) + } + // Empty-starved session — no events, started 2h ago. + if _, err := pool.Exec(ctx, ` + INSERT INTO play_sessions (user_id, started_at, last_event_at, track_count) + VALUES ($1, now() - interval '2 hours', now() - interval '2 hours', 0) + `, userID); err != nil { + t.Fatalf("seed empty session: %v", err) + } + // Active session — recent last_event_at, has events. + if _, err := pool.Exec(ctx, ` + INSERT INTO play_sessions (user_id, started_at, last_event_at, track_count) + VALUES ($1, now() - interval '30 minutes', now() - interval '5 minutes', 3) + `, userID); err != nil { + t.Fatalf("seed active session: %v", err) + } + + w.tickOnce(ctx) + + var closedCount, openCount int + if err := pool.QueryRow(ctx, ` + SELECT count(*) FROM play_sessions + WHERE user_id = $1 AND ended_at IS NOT NULL + `, userID).Scan(&closedCount); err != nil { + t.Fatalf("count closed: %v", err) + } + if err := pool.QueryRow(ctx, ` + SELECT count(*) FROM play_sessions + WHERE user_id = $1 AND ended_at IS NULL + `, userID).Scan(&openCount); err != nil { + t.Fatalf("count open: %v", err) + } + if closedCount != 2 { + t.Errorf("closed sessions = %d, want 2 (idle + empty-starved)", closedCount) + } + if openCount != 1 { + t.Errorf("open sessions = %d, want 1 (active)", openCount) + } +} + +func TestGcResetStuckSystemPlaylistRuns(t *testing.T) { + w, pool := testWorker(t) + ctx := context.Background() + stuckUser := seedUser(t, pool, "stuck") + activeUser := seedUser(t, pool, "active") + + // Stuck row: in_flight, last_run_at 30 min ago. + if _, err := pool.Exec(ctx, ` + INSERT INTO system_playlist_runs (user_id, last_run_at, in_flight) + VALUES ($1, now() - interval '30 minutes', true) + `, stuckUser); err != nil { + t.Fatalf("seed stuck run: %v", err) + } + // Active row: in_flight, last_run_at 2 min ago — still legitimate. + if _, err := pool.Exec(ctx, ` + INSERT INTO system_playlist_runs (user_id, last_run_at, in_flight) + VALUES ($1, now() - interval '2 minutes', true) + `, activeUser); err != nil { + t.Fatalf("seed active run: %v", err) + } + + w.tickOnce(ctx) + + var stuckFlight, activeFlight bool + if err := pool.QueryRow(ctx, ` + SELECT in_flight FROM system_playlist_runs WHERE user_id = $1 + `, stuckUser).Scan(&stuckFlight); err != nil { + t.Fatalf("read stuck row: %v", err) + } + if err := pool.QueryRow(ctx, ` + SELECT in_flight FROM system_playlist_runs WHERE user_id = $1 + `, activeUser).Scan(&activeFlight); err != nil { + t.Fatalf("read active row: %v", err) + } + if stuckFlight { + t.Errorf("stuck row still in_flight after sweep") + } + if !activeFlight { + t.Errorf("active row was reset (should be left alone — only 2 min old)") + } +} + +func TestGcDeleteExpiredPasswordResets(t *testing.T) { + w, pool := testWorker(t) + ctx := context.Background() + userID := seedUser(t, pool, "pwd") + + // Expired-unused (> 1h past expires_at) — should delete. + if _, err := pool.Exec(ctx, ` + INSERT INTO password_resets (token, user_id, expires_at) + VALUES ('expired-old', $1, now() - interval '2 hours') + `, userID); err != nil { + t.Fatalf("seed expired: %v", err) + } + // Used (> 7 days past used_at) — should delete. + if _, err := pool.Exec(ctx, ` + INSERT INTO password_resets (token, user_id, expires_at, used_at) + VALUES ('used-old', $1, now() - interval '8 days', now() - interval '8 days') + `, userID); err != nil { + t.Fatalf("seed used-old: %v", err) + } + // Active (expires in future) — should survive. + if _, err := pool.Exec(ctx, ` + INSERT INTO password_resets (token, user_id, expires_at) + VALUES ('active', $1, now() + interval '1 hour') + `, userID); err != nil { + t.Fatalf("seed active: %v", err) + } + // Recently used (< 7 days) — should survive for audit. + if _, err := pool.Exec(ctx, ` + INSERT INTO password_resets (token, user_id, expires_at, used_at) + VALUES ('used-recent', $1, now() - interval '1 hour', now() - interval '1 day') + `, userID); err != nil { + t.Fatalf("seed used-recent: %v", err) + } + + w.tickOnce(ctx) + + var remaining []string + rows, err := pool.Query(ctx, `SELECT token FROM password_resets WHERE user_id = $1`, userID) + if err != nil { + t.Fatalf("list remaining: %v", err) + } + defer rows.Close() + for rows.Next() { + var tok string + if err := rows.Scan(&tok); err != nil { + t.Fatalf("scan: %v", err) + } + remaining = append(remaining, tok) + } + wantSet := map[string]bool{"active": true, "used-recent": true} + if len(remaining) != len(wantSet) { + t.Errorf("remaining tokens = %v, want %v", remaining, []string{"active", "used-recent"}) + } + for _, tok := range remaining { + if !wantSet[tok] { + t.Errorf("unexpected surviving token %q", tok) + } + } +} + +// Verifies tickOnce doesn't blow up when the tables are completely +// empty — sweeps just no-op. Regression guard against an EXEC vs +// QUERY-row-count mismatch failing on zero rows. +func TestGcTickOnce_NoOpOnEmptyTables(t *testing.T) { + w, _ := testWorker(t) + w.tickOnce(context.Background()) +} + +// Sanity check on the Run() loop's cancel behaviour — we don't want +// to leave a goroutine spinning at test-runner exit. 10ms tick with +// an immediate cancel should return promptly. +func TestGcRun_HonoursContextCancel(t *testing.T) { + w, _ := testWorker(t) + w.tick = 10 * time.Millisecond + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + w.Run(ctx) + close(done) + }() + cancel() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("Run did not return after cancel") + } +} diff --git a/internal/library/delete.go b/internal/library/delete.go index 532f9727..f496871e 100644 --- a/internal/library/delete.go +++ b/internal/library/delete.go @@ -29,9 +29,12 @@ var ErrTrackNotFound = errors.New("library: track not found") // // Order matters: file first, then DB. If the file delete fails (permission, // I/O error), we leave the DB row alone so the admin can retry. The reverse -// failure mode — file gone, DB row still present — is recoverable: the next -// library scan reconciles missing files by removing their tracks rows. So -// the function is retry-safe rather than atomic, by design. +// failure mode — file gone, DB row still present — is currently NOT +// auto-reconciled (drift #572 audit found the misleading prior claim +// that a scan would clean it up — the scanner only walks + upserts; +// it does not enumerate orphan rows). An admin must re-trigger +// DeleteTrackFile or delete the row manually. A scanrun orphan-row +// sweep is tracked as future work in the audit queue. func DeleteTrackFile(ctx context.Context, pool *pgxpool.Pool, trackID pgtype.UUID) error { q := dbq.New(pool) track, err := q.GetTrackByID(ctx, trackID) diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 5f47912a..c6b7c6e7 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -23,13 +23,20 @@ import ( syncpkg "git.fabledsword.com/bvandeusen/minstrel/internal/sync" ) -// audioExtensions is the v1 set. Duration extraction is not wired, so all of -// these get duration_ms=0 until an ffprobe / native-decoder pass lands. +// audioExtensions is the set the scanner indexes. Keep in sync with +// `internal/api/media.go` MIME detection — the stream handler must be +// able to serve every extension the scanner indexes, and there is no +// point in adding extensions to the stream handler that the scanner +// will silently skip. Drift #571 caught the divergence after .opus, +// .aac, and .wav were added to media.go but not here. var audioExtensions = map[string]bool{ ".mp3": true, ".m4a": true, ".flac": true, ".ogg": true, + ".opus": true, + ".aac": true, + ".wav": true, } type Stats struct { diff --git a/internal/playevents/writer.go b/internal/playevents/writer.go index f523a3b4..9759d578 100644 --- a/internal/playevents/writer.go +++ b/internal/playevents/writer.go @@ -69,11 +69,32 @@ func (w *Writer) RecordPlayStarted( } // systemPlaylistSources are the play_events.source values that count -// against a system playlist's rotation (Fable #415). Add future -// system-playlist kinds here as they ship (deep_cuts, rediscover, …). +// against a system playlist's rotation (Fable #415). Mirrors the +// `playlists_kind_variant_consistent` CHECK in migration +// 0028_discovery_mix_variants.up.sql — every variant in that CHECK +// list that ships as a refreshable system mix needs to be here so +// the rotation reporter sees Android + web plays from that surface. +// +// Drift #563: this map drifted behind the migrations. It had only +// for_you + discover but migrations 0021 + 0028 added 6 more +// variants. Plays from Rediscover / Deep Cuts / Songs Like X / +// New for You / On This Day / First Listens didn't advance the +// per-user rotation, so "unplayed first" ordering staled on those +// mixes — same tracks kept surfacing. +// +// Future variant additions should update this map AND the CHECK in +// the matching migration in the same change; the +// `db_check_constraint_for_new_variants` standing rule covers the +// CHECK half. var systemPlaylistSources = map[string]bool{ - "for_you": true, - "discover": true, + "for_you": true, + "discover": true, + "deep_cuts": true, + "rediscover": true, + "new_for_you": true, + "on_this_day": true, + "first_listens": true, + "songs_like_artist": true, } // RecordPlayStartedWithSource is RecordPlayStarted plus a `source` diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index 26796f06..d280e802 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -106,7 +106,13 @@ export type LikedIdsResponse = { }; export type EventRequest = - | { type: 'play_started'; track_id: string; client_id?: string } + // `source` is the system-playlist variant the play came from + // (for_you, discover, deep_cuts, …); empty/undefined for library + // / user-playlist / radio / Subsonic. Server stores it on + // play_events.source and uses it to advance the rotation. Drift + // #555: the type was missing `source` so callers had no typed slot + // for it and a "clean up extra properties" refactor could drop it. + | { type: 'play_started'; track_id: string; client_id?: string; source?: string } | { type: 'play_ended'; play_event_id: string; duration_played_ms: number } | { type: 'play_skipped'; play_event_id: string; position_ms: number }; diff --git a/web/src/lib/auth/publicRoutes.test.ts b/web/src/lib/auth/publicRoutes.test.ts index 1210eca8..71e875e3 100644 --- a/web/src/lib/auth/publicRoutes.test.ts +++ b/web/src/lib/auth/publicRoutes.test.ts @@ -30,4 +30,22 @@ describe('isPublicRoute', () => { expect(isPublicRoute('/login/extra')).toBe(false); expect(isPublicRoute('/loginx')).toBe(false); }); + + // Regression guard for drift #558: /forgot-password and the + // /reset-password/ deep links must be reachable without a + // session — the reset flow is entered by clicking an email link + // while signed out, so the auth gate redirecting to /login broke + // account recovery. + test('/forgot-password is public', () => { + expect(isPublicRoute('/forgot-password')).toBe(true); + }); + + test('/reset-password/ is public', () => { + expect(isPublicRoute('/reset-password/abc-123')).toBe(true); + expect(isPublicRoute('/reset-password/')).toBe(true); + }); + + test('/reset-password (no trailing slash) is NOT public — only the token sub-path', () => { + expect(isPublicRoute('/reset-password')).toBe(false); + }); }); diff --git a/web/src/lib/auth/publicRoutes.ts b/web/src/lib/auth/publicRoutes.ts index e79a9c31..aaecf98f 100644 --- a/web/src/lib/auth/publicRoutes.ts +++ b/web/src/lib/auth/publicRoutes.ts @@ -2,8 +2,15 @@ // when the visitor is unauthenticated. Bootstrap-admin self-registration // (#376) requires /register to be reachable without a session — without // /register here, the login page's "Register" link bounces back to /login. -const PUBLIC_ROUTES = new Set(['/login', '/register']); +// +// Drift #558: /forgot-password and /reset-password/ were missing +// from this set. The email-link reset flow is by definition entered +// without a session — a signed-out user clicking their reset link was +// being bounced to /login, breaking account recovery. +const PUBLIC_ROUTES = new Set(['/login', '/register', '/forgot-password']); +const PUBLIC_PREFIXES = ['/reset-password/']; export function isPublicRoute(pathname: string): boolean { - return PUBLIC_ROUTES.has(pathname); + if (PUBLIC_ROUTES.has(pathname)) return true; + return PUBLIC_PREFIXES.some((p) => pathname.startsWith(p)); } diff --git a/web/src/lib/player/store.svelte.ts b/web/src/lib/player/store.svelte.ts index 2a7577d3..42650296 100644 --- a/web/src/lib/player/store.svelte.ts +++ b/web/src/lib/player/store.svelte.ts @@ -462,7 +462,18 @@ $effect.root(() => { _radioRefreshInFlight = true; const seed = _radioSeedId; - const exclude = _queue.map((t) => t.id).join(','); + // Drift #554: cap the exclude list so a multi-hour radio session + // doesn't grow the query string past common 8KB limits and start + // 414-ing /api/radio. The .catch() below would silently swallow + // that failure and the player would stop topping up — a dead + // radio. The server's RecentlyPlayedHours filter already handles + // broader history dedup, so the request-side exclude only needs + // to cover the visible queue's recent tail. + const RADIO_EXCLUDE_CAP = 100; + const exclude = _queue + .slice(-RADIO_EXCLUDE_CAP) + .map((t) => t.id) + .join(','); api .get( `/api/radio?seed_track=${encodeURIComponent(seed)}&exclude=${exclude}` diff --git a/web/src/routes/admin/playback-errors/+page.svelte b/web/src/routes/admin/playback-errors/+page.svelte index 86fdf9a0..d43993f0 100644 --- a/web/src/routes/admin/playback-errors/+page.svelte +++ b/web/src/routes/admin/playback-errors/+page.svelte @@ -274,7 +274,7 @@ {/if} diff --git a/web/src/routes/library/+page.server.ts b/web/src/routes/library/+page.ts similarity index 60% rename from web/src/routes/library/+page.server.ts rename to web/src/routes/library/+page.ts index 11a5e992..a5547a20 100644 --- a/web/src/routes/library/+page.server.ts +++ b/web/src/routes/library/+page.ts @@ -4,6 +4,11 @@ import { redirect } from '@sveltejs/kit'; // always renders the active sub-page. Bare hits go to the default tab // (Artists, matching Android's LibraryScreen default). 308 = permanent // + preserve method, so SPA navigations and direct loads behave the same. +// +// Drift #559: this was a +page.server.ts which DOES NOT run in +// production — the app is configured as adapter-static + ssr=false +// (see +layout.ts). +page.ts (universal load) runs client-side, which +// is what the SPA actually executes when a route is hit. export const load = () => { throw redirect(308, '/library/artists'); }; diff --git a/web/src/routes/playlists/+page.server.ts b/web/src/routes/playlists/+page.ts similarity index 69% rename from web/src/routes/playlists/+page.server.ts rename to web/src/routes/playlists/+page.ts index a425747b..d506d638 100644 --- a/web/src/routes/playlists/+page.server.ts +++ b/web/src/routes/playlists/+page.ts @@ -5,6 +5,10 @@ import { redirect } from '@sveltejs/kit'; // still lives here at /routes/playlists/[id]/+page.svelte — that URL // matches the server API shape and is unchanged. // 308 = permanent + preserve method; old bookmarks land on the new URL. +// +// Drift #559: this was a +page.server.ts which DOES NOT run in +// production (adapter-static + ssr=false, see +layout.ts). +page.ts +// universal load runs client-side and IS what the SPA executes. export const load = () => { throw redirect(308, '/library/playlists'); };