Merge pull request 'Drift audit 2026-06-02 — 26 findings shipped' (#75) from dev into main
test-go / test (push) Successful in 43s
test-web / test (push) Successful in 56s
android / Build + lint + test (push) Successful in 5m47s
release / Build signed APK (tag releases only) (push) Successful in 5m10s
release / Build + push container image (push) Successful in 24s
test-go / integration (push) Failing after 12m37s

This commit was merged in pull request #75.
This commit is contained in:
2026-06-02 19:21:53 -04:00
29 changed files with 1104 additions and 88 deletions
@@ -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()
@@ -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<CachedLikeEntity>) {
clearForUser(userId)
upsertAll(rows)
}
}
@@ -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:<variant>" — 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()
}
@@ -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<List<ArtistRef>> =
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<List<AlbumRef>> =
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<List<TrackRef>> =
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<Boolean> =
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<String> =
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<CachedLikeEntity>()
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"
@@ -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<PlaybackErrorEvent>(Channel.CONFLATED)
private val playbackErrorEventsChannel = Channel<PlaybackErrorEvent>(Channel.BUFFERED)
val playbackErrorEvents: Flow<PlaybackErrorEvent> = playbackErrorEventsChannel.receiveAsFlow()
/**
@@ -93,10 +102,30 @@ class PlayerController @Inject constructor(
*/
private var queueRefs: List<TrackRef> = 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<Unit>()
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<TrackRef>, initialIndex: Int = 0, source: String? = null) {
fun setQueue(
tracks: List<TrackRef>,
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 {
@@ -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
@@ -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:<id>" 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)
}
}
@@ -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,
) {
@@ -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)
}
}
+11
View File
@@ -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.
+28 -9
View File
@@ -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)
+17 -7
View File
@@ -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))
}
+50 -1
View File
@@ -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) {
+8 -5
View File
@@ -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
+126
View File
@@ -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
}
+8 -5
View File
@@ -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
+70
View File
@@ -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');
+101
View File
@@ -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)
}
}
+320
View File
@@ -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")
}
}
+6 -3
View File
@@ -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)
+9 -2
View File
@@ -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 {
+25 -4
View File
@@ -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`
+7 -1
View File
@@ -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 };
+18
View File
@@ -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/<token> 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/<token> 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);
});
});
+9 -2
View File
@@ -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/<token> 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));
}
+12 -1
View File
@@ -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<RadioResponse>(
`/api/radio?seed_track=${encodeURIComponent(seed)}&exclude=${exclude}`
@@ -274,7 +274,7 @@
<button
type="button"
onclick={confirmDeleteFile}
class="rounded bg-action-danger px-3 py-2 text-sm text-action-fg hover:opacity-90"
class="rounded bg-action-destructive px-3 py-2 text-sm text-action-fg hover:opacity-90"
>Delete file</button>
</div>
{/if}
@@ -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');
};
@@ -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');
};