From 47d2f61161a4e4acfed378f672180c5522a50148 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:11:25 -0400 Subject: [PATCH 01/11] =?UTF-8?q?fix:=20drift=20audit=20batch=201=20?= =?UTF-8?q?=E2=80=94=20six=20small=20mechanical=20wins?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from the 2026-06-02 multi-system drift audit (Scribe parent task #552): - **#553 (web)** Tailwind class fix: web admin playback-errors Delete confirm button was using `bg-action-danger`, an undefined token — swap to `bg-action-destructive` to match every other destructive button. Restored the Oxblood signal that distinguishes Delete from Cancel. - **#555 (web)** Type the `source` field on `play_started` in the EventRequest discriminated union. Server's eventRequest accepts it; web's TS type was missing the slot, so a "drop extra properties" refactor could silently strip the source tag and break system- playlist rotation attribution. - **#556 + #557 (server)** Coverage rollup whitelist was pinned to ('sidecar','embedded','mbcaa','theaudiodb'); migration 0020 added 'deezer' and 'lastfm' as valid cover_art_source values but those never got wired in, so albums with art from those providers silently counted as MISSING in the admin Coverage dashboard. The rollup test was seeding only the pre-0020 sources, masking the gap in CI. Extend the query to include deezer + lastfm; seed the test with one row per valid source (regression-guards future additions). - **#558 (web)** Auth gate was blocking /forgot-password and /reset-password/ — both are entered without a session by definition, so the email-link reset flow was bouncing signed-out users to /login. Add /forgot-password to the public set and a /reset-password/ prefix matcher. New tests assert both routes reach their pages without redirect. - **#571 (server)** Library scanner was indexing only .mp3/.m4a/.flac /.ogg while the stream handler (media.go) had been extended to serve .opus, .aac, and .wav. A user with .opus files in their library never saw them in artist/album listings because the scanner skipped indexing — silent data loss. Aligned the scanner to match the media handler. Scribe statuses updated to in_progress; flipping to done after the push since these are mechanical and verified directly against the cited file:lines. --- internal/api/admin_coverage_test.go | 37 ++++++++++++++----- internal/db/dbq/albums.sql.go | 13 ++++--- internal/db/queries/albums.sql | 13 ++++--- internal/library/scanner.go | 11 +++++- web/src/lib/api/types.ts | 8 +++- web/src/lib/auth/publicRoutes.test.ts | 18 +++++++++ web/src/lib/auth/publicRoutes.ts | 11 +++++- .../routes/admin/playback-errors/+page.svelte | 2 +- 8 files changed, 88 insertions(+), 25 deletions(-) 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/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/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/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/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/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} -- 2.52.0 From fb3116d64042ccbe993ce75f33065493091f20bb Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:16:29 -0400 Subject: [PATCH 02/11] =?UTF-8?q?fix:=20drift=20audit=20batch=202=20?= =?UTF-8?q?=E2=80=94=20patterned=20fixes=20mirroring=20prior=20work?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings + one cancelled duplicate from the 2026-06-02 drift audit (Scribe parent task #552): - **#561 (Android)** PlayerController.playbackErrorEventsChannel was Channel.CONFLATED. The PlaybackErrorReporter coroutine reads it in a debounce loop that buffers events to coalesce into "Skipped N unplayable tracks" — but CONFLATED silently dropped every emission except the latest each time the reader wasn't actively pulling. A network blip that failed 5 tracks back-to-back surfaced only the last failure to the snackbar AND only POSTed one playback_errors row to the admin inbox. Switch to BUFFERED (default capacity 64, well above any plausible burst rate). Coalescing path now reaches N > 1 and the admin inbox sees every failure. - **#563 (server)** systemPlaylistSources rotation whitelist in playevents/writer.go had drifted behind the migrations. It listed only for_you + discover; migrations 0021 + 0028 added 6 more variants (deep_cuts, rediscover, new_for_you, on_this_day, first_listens, songs_like_artist) that ship as refreshable system mixes. Plays from those surfaces never advanced the per-user rotation, so "unplayed first" ordering staled — the same tracks kept resurfacing. Add all 6 to the map; comment now points at the migration's CHECK list as the canonical source so future variants notice the requirement. #573 was the duplicate auditor hit for the same drift; cancelled in Scribe. - **#564 (Android)** Android emitted source = "playlist:" for system-mix plays from Home and PlaylistDetail, but the server's rotation matcher keys on the BARE variant string (web sends the bare form — PlaylistCard.svelte:83). Misalignment meant system-mix plays from Android never advanced rotation; switching from web to Android effectively reset the perceived "unplayed next" ordering. Fix HomeScreen.kt:291 to send bare variant and PlaylistDetailScreen.kt's play() to prefer systemVariant over the playlist: tag when the playlist is a refreshable system mix. User playlists keep playlist: (intentional — rotation only applies to system mixes anyway). - **#568 + #569 (Android)** AuthCookieInterceptor was unconditionally attaching the Minstrel session cookie to every outgoing request AND wiping the session on any 401. The shared OkHttpClient is also used by Coil for external image fetches (artwork.musicbrainz.org, coverartarchive.org, Lidarr /MediaCover URLs); this leaked the session cookie to those hosts (privacy posture) AND silently signed users out of Minstrel if any external image host returned 401. Scope both attach + clear to the placeholder.invalid sentinel host the same way BaseUrlInterceptor was scoped in aec10ce7. Two new regression tests cover the external-host pass-through. Existing tests rewritten to make requests through the placeholder URL so they exercise the in-scope path explicitly. All five Scribe tasks updated to in_progress at start, will flip to done after CI green on this push. --- .../minstrel/api/AuthCookieInterceptor.kt | 27 +++++++-- .../minstrel/home/ui/HomeScreen.kt | 10 +++- .../minstrel/player/PlayerController.kt | 15 ++++- .../playlists/ui/PlaylistDetailScreen.kt | 11 +++- .../minstrel/api/AuthCookieInterceptorTest.kt | 55 +++++++++++++++++-- internal/playevents/writer.go | 29 ++++++++-- 6 files changed, 130 insertions(+), 17 deletions(-) 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/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/player/PlayerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayerController.kt index 31dac4b2..9fc44bb7 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() /** 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/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/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` -- 2.52.0 From b19c6217436ff123917457269f320924276fbe52 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:18:59 -0400 Subject: [PATCH 03/11] =?UTF-8?q?fix:=20drift=20audit=20batch=203a=20?= =?UTF-8?q?=E2=80=94=20Android=20offline=20correctness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the 2026-06-02 drift audit (Scribe parent #552): - **#577 (Android)** RequestsViewModel.cancel() called refresh() on BOTH Synced and Queued outcomes. Synced is fine (re-fetch the canonical list); Queued is offline by definition — the optimistic removal at the top of cancel() is already correct, and refresh() on Queued either (a) gets the not-yet-delivered cancelled row back from the server and snaps it into the list (confusing), or (b) fails with a transport error and flips the screen to UiState.Error so the user thinks the cancel failed even though it's queued. Gate refresh() on outcome == Synced; the mutation replayer reconciles when connectivity returns. - **#570 (Android)** LikesRepository.refreshIds() pulled the server's likes list and INSERTed it into cached_likes — but never DELETEd local rows the server no longer surfaces. A cross-device unlike (user likes on web, then unlikes on web) left the entry visible on Android's Liked tab indefinitely with no way to clear short of wiping app data. Add CachedLikeDao.clearForUser + a @Transaction replaceAllForUser that atomically wipes-then-inserts the user's set; refreshIds() uses replaceAllForUser so the local cache is exactly what the server reports. The LOCAL_USER_ID hardcode is its own drift (#576) and stays for now — fixing it needs threading AuthStore.userId through the repo. --- .../minstrel/cache/db/dao/CachedLikeDao.kt | 18 +++++++++++++++ .../minstrel/likes/data/LikesRepository.kt | 19 ++++++++++----- .../minstrel/requests/ui/RequestsViewModel.kt | 23 +++++++++++++------ 3 files changed, 47 insertions(+), 13 deletions(-) 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/likes/data/LikesRepository.kt b/android/app/src/main/java/com/fabledsword/minstrel/likes/data/LikesRepository.kt index 1e5087ed..0151a506 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 @@ -111,11 +111,18 @@ 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 wire = api.ids() @@ -123,7 +130,7 @@ class LikesRepository @Inject constructor( 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) + likeDao.replaceAllForUser(LOCAL_USER_ID, rows) } companion object { 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, ) { -- 2.52.0 From 5014f7548ec396f25ecb9288944a671d0b3a1761 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:21:39 -0400 Subject: [PATCH 04/11] =?UTF-8?q?fix:=20drift=20audit=20batch=203b=20?= =?UTF-8?q?=E2=80=94=20cold-boot=20resume=20correctness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related findings from the 2026-06-02 drift audit (#552): - **#560 (Android)** PlayerController.setQueue() unconditionally called controller.play() at the end, with no way for ResumeController to opt out. Cold-boot resume therefore restored the persisted queue AND auto-started playback, which surprised users who had paused mid-track before backgrounding the app. Add `autoplay: Boolean = true` parameter; ResumeController passes false. Every existing setQueue call site continues to autoplay (the default is unchanged). - **#562 (Android)** ResumeController.restore() ran from MinstrelApplication.onCreate alongside PlayerController's own init {} block that asynchronously binds the MediaController to MinstrelPlayerService. On fast devices with slow IPC the restore could land before mediaController was non-null; PlayerController.setQueue early-returns on null mediaController, so the restored queue was silently dropped — the user would open the app to an empty player after explicitly using "resume previous queue". Add `awaitReady()` suspend that completes when the MediaController binding lands; ResumeController awaits it before calling setQueue. The two fixes ship together because the autoplay opt-out only matters once the await fix guarantees the queue actually reaches the player. --- .../minstrel/player/PlayerController.kt | 45 +++++++++++++++++-- .../minstrel/player/ResumeController.kt | 10 +++++ 2 files changed, 52 insertions(+), 3 deletions(-) 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 9fc44bb7..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 @@ -102,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() } @@ -144,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() } /** @@ -282,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 -- 2.52.0 From b970b873433208e1061a7962463058bc42a9d189 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:24:23 -0400 Subject: [PATCH 05/11] =?UTF-8?q?fix(server):=20drift=20#578=20=E2=80=94?= =?UTF-8?q?=20/api/me=20returns=20profile=20shape=20with=20display=5Fname?= =?UTF-8?q?=20+=20email?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server-side fix for the drift audit finding (Scribe #578, parent #552). Mirrored on Android (and Flutter) but the root cause and the smallest blast-radius fix both live here. The bug: - Android MeApi.getProfile() calls GET /api/me and deserializes into MyProfileWire which has nullable display_name + email. - Server's handleGetMe was emitting the narrower UserView shape (id, username, is_admin only). - Android always saw displayName=null, email=null. The Settings → Profile screen rendered BLANK form fields for users with stored values. - Saving from the blank state submitted empty strings to PUT /api/me/profile, which interprets empty as "clear to NULL" (me_profile.go:53-65) — DESTROYING the user's saved profile. - Flutter (flutter_client/lib/api/endpoints/settings.dart:9-12) has the identical bug pattern. The fix: - handleGetMe now emits profileViewFromUser(user) — the same shape PUT /api/me/profile already returns (meProfileResp: id, username, display_name, email, is_admin). - auth.UserFromContext already returns a full dbq.User row, so no extra DB lookup needed. - Web's User TypeScript type is narrower than this response but doesn't care about the extra fields (TS structural typing). - LoginResp.User still uses UserView; login response unchanged. New test asserts the regression directly: a user with stored display_name + email sees them in /api/me. Old test updated to decode into meProfileResp and assert the nullable fields are correctly null for an unset profile. Android side needs no change — the existing wire shape already expected display_name + email; this just delivers them. --- internal/api/me.go | 24 +++++++++++++------ internal/api/me_test.go | 51 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 8 deletions(-) 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) { -- 2.52.0 From 413d729711625fa187c8101f79f2d9cd068fe805 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:25:52 -0400 Subject: [PATCH 06/11] =?UTF-8?q?fix(web):=20drift=20audit=20batch=205=20?= =?UTF-8?q?=E2=80=94=20SSR=20redirect=20+=20radio=20exclude=20cap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two web-side findings from the 2026-06-02 drift audit (#552): - **#559** /library and /playlists each had a +page.server.ts file calling redirect(308, ...). The app is configured as adapter-static + ssr=false (+layout.ts:5), so +page.server.ts files only run at build time / dev server — NEVER at runtime in the deployed build. Direct navigation to /library or /playlists (mobile bookmarks, hand-typed URLs) hit a blank page or 404. We worked around this earlier today by linking the nav directly to /library/artists, but bookmarks stayed broken. Converted both files to +page.ts (universal load) — same redirect logic, runs client-side in the SPA, which is what actually executes. - **#554** Radio auto-refresh built its exclude= query parameter from the ENTIRE queue, growing unbounded each refresh as new tracks were appended. UUIDs are ~36 chars + comma; with the common 8KB query-string limit, ~220 tracks is the ceiling. A multi-hour radio session eventually 414'd; the .catch() ate the error and the player silently stopped topping up — dead radio with no user-visible signal. Cap exclude to the most recent 100 ids; the server's RecentlyPlayedHours filter already handles broader history dedup so the request-side cap only needs to cover the visible queue's recent tail. --- web/src/lib/player/store.svelte.ts | 13 ++++++++++++- .../routes/library/{+page.server.ts => +page.ts} | 5 +++++ .../routes/playlists/{+page.server.ts => +page.ts} | 4 ++++ 3 files changed, 21 insertions(+), 1 deletion(-) rename web/src/routes/library/{+page.server.ts => +page.ts} (60%) rename web/src/routes/playlists/{+page.server.ts => +page.ts} (69%) 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/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'); }; -- 2.52.0 From bda0896d82c2d2a71c29bb44554902d13cd74cde Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:26:45 -0400 Subject: [PATCH 07/11] =?UTF-8?q?docs(server):=20drift=20#572=20=E2=80=94?= =?UTF-8?q?=20delete.go=20honest=20about=20missing=20reconcile?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docstring claimed "the next library scan reconciles missing files by removing their tracks rows" — but scanner.go only does filepath.WalkDir + UpsertTrack; it never enumerates existing rows to check file_path presence, and it never DELETEs orphan rows. The audit verified this — repo-wide grep finds no orphan-sweep code. The lie is load-bearing: lidarrquarantine/service.go:270 leans on this guarantee, so downstream code thinks the orphan case heals itself. Fix the comment to state reality (admin re-trigger or manual cleanup) and reference the open follow-up for adding a real sweep. The actual reconcile pass is a separate piece of work (needs scanrun integration + retention semantics + tests) and stays in the Scribe audit queue. --- internal/library/delete.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) 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) -- 2.52.0 From 258bc1f75c7ca05d3032fa89d5c6d2d37dadd21c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:32:22 -0400 Subject: [PATCH 08/11] =?UTF-8?q?feat(server):=20drift=20audit=20batch=207?= =?UTF-8?q?=20=E2=80=94=20periodic=20GC=20worker=20for=205=20lifecycle=20g?= =?UTF-8?q?aps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New `internal/gc` package with a single Worker that runs all five lifecycle / retention sweeps from the 2026-06-02 drift audit on a 1-hour tick. Each sweep is small, idempotent (re-running on already-clean rows is a no-op), and logs its affected-row count. Sweeps (Scribe parent #552): - **#566** GcCloseStalePlayEvents — play_events rows opened > 24h ago that never got a play_ended (client crash, network drop). Synthesizes ended_at from duration_played_ms when known, falls back to now() so the row stops looking "open" to downstream filters (ended_at IS NULL). - **#565** GcClosePlaySessionsWithNoRecentEvents — play_sessions with last_event_at older than 6h get ended_at = last_event_at ("user moved on"); empty sessions older than 1h get closed too (stale handshakes from clients that never recorded a play). The audit caught that the column was added but never populated by any writer — every session row was "open" forever, breaking downstream dedup queries that assume closed semantics. - **#567** GcExpireScrobbleQueueFailedRows — drops scrobble_queue rows in status='failed' older than 14 days. The worker stops retrying after maxAttempts so these otherwise accumulate forever on a persistent ListenBrainz outage / revoked token. - **#574** GcResetStuckSystemPlaylistRuns — flips system_playlist_runs.in_flight back to false on rows whose last_run_at is older than 10 minutes. Catches goroutine-panic wedges where the generator died 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 would otherwise deadlock all future regens for that user. Records "stuck-row auto-reset by gc" in last_error so the operator can tell auto-reset from a recent real failure. - **#575** GcDeleteExpiredPasswordResets — deletes expired password_resets rows. Unused expired rows go after a 1h grace (gives the operator time to debug an active reset attempt); used rows are kept 7 days for audit. Wiring: - main.go `go gcWorker.Run(ctx)` alongside the other periodic workers (scrobble, similarity, lidarr). - tickOnce fires once at start so a freshly-deployed server does its initial sweep without waiting a full tick, matching the scrobble worker pattern. - Errors per sweep are logged but do NOT abort the remaining ones — a transient pgx error from one query shouldn't prevent the others from running. Tests: - 4 integration tests, one per UPDATE/DELETE sweep, that seed rows-to-sweep + rows-to-leave-alone and assert the right rows changed state. Skip unless MINSTREL_TEST_DATABASE_URL is set (mirrors the api package pattern). - Empty-tables no-op smoke test. - Run() cancellation honoured (no spinning goroutine at test-runner exit). That's all five remaining server-side lifecycle findings from the audit. The Android LOCAL_USER_ID hardcode (#576) is a separate refactor that needs auth-store wiring and stays in the queue. --- cmd/minstrel/main.go | 11 ++ internal/db/dbq/gc.sql.go | 126 +++++++++++++++ internal/db/queries/gc.sql | 70 ++++++++ internal/gc/worker.go | 101 ++++++++++++ internal/gc/worker_test.go | 317 +++++++++++++++++++++++++++++++++++++ 5 files changed, 625 insertions(+) create mode 100644 internal/db/dbq/gc.sql.go create mode 100644 internal/db/queries/gc.sql create mode 100644 internal/gc/worker.go create mode 100644 internal/gc/worker_test.go 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/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/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..eb7ef9ee --- /dev/null +++ b/internal/gc/worker_test.go @@ -0,0 +1,317 @@ +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 + if err := pool.QueryRow(ctx, ` + INSERT INTO artists (name) VALUES ('A') RETURNING id + `).Scan(new(pgtype.UUID)); err != nil { + t.Fatalf("seed artist row: %v", err) + } + // Get the just-inserted artist id. + var artistID pgtype.UUID + if err := pool.QueryRow(ctx, `SELECT id FROM artists WHERE name = 'A'`).Scan(&artistID); err != nil { + t.Fatalf("read artist: %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) + } + if err := pool.QueryRow(ctx, ` + INSERT INTO tracks (album_id, artist_id, title, file_path, duration_ms) + VALUES ($1, $2, 'T', '/x.mp3', 180000) 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") + } +} -- 2.52.0 From dbcadf0f93559562bfb0d8f8617ae630eee91e47 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:35:54 -0400 Subject: [PATCH 09/11] =?UTF-8?q?fix(android):=20drift=20#576=20=E2=80=94?= =?UTF-8?q?=20LikesRepository=20uses=20real=20userId,=20clears=20on=20swit?= =?UTF-8?q?ch?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final drift audit finding (Scribe parent #552). LikesRepository hardcoded LOCAL_USER_ID = "local" as the cached_likes discriminator since before the auth slice landed. After auth shipped, the app has a real per-user session but every device wrote rows under the same "local" bucket — so sharing an Android device between two Minstrel accounts left the previous user's likes visible to the new user. Changes: - Inject AuthController + ApplicationScope so the repo can read the current user UUID and subscribe to user-switch events. - `currentUserId()` resolves the cached_likes discriminator to `authController.currentUser.value?.id` with the legacy "local" fallback (ANONYMOUS_USER_ID, renamed from LOCAL_USER_ID) so pre-#576 cache rows from existing installs stay queryable until the first authenticated refreshIds() overwrites them. - All eight call sites that used the constant now use the helper: observeLikedArtists/Albums/Tracks, observeIsLiked, likedTrackIds, toggleLike (optimistic upsert + delete), refreshIds (server replace). - init {} subscribes to authController.currentUser; when the signed-in id changes, the OUTGOING user's rows get likeDao.clearForUser. Mostly a hygiene fix — the discriminator already prevents the wrong user from SEEING leaked rows, but without this they pile up forever as different accounts sign in/out on the same device. This closes the final drift audit finding from the 2026-06-02 run. 26 of 26 candidate findings either confirmed-and-shipped (24) or cancelled-as-duplicate (1) or shipped-with-honest-doc-fix (1). --- .../minstrel/likes/data/LikesRepository.kt | 85 ++++++++++++++----- 1 file changed, 66 insertions(+), 19 deletions(-) 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 0151a506..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) @@ -125,17 +165,24 @@ class LikesRepository @Inject constructor( * 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.replaceAllForUser(LOCAL_USER_ID, 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" -- 2.52.0 From 305d4780ac945421a767f737a4a6ae0d13c989af Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 18:47:31 -0400 Subject: [PATCH 10/11] fix(server): TestGcCloseStalePlayEvents seeds artist with sort_name The artists table requires sort_name (NOT NULL constraint added by 0009_artist_sort.up.sql). My GC integration test was inserting only name + relying on a separate SELECT to pull the id back, which both (a) violated the NOT NULL constraint and (b) was unnecessarily indirect. RETURNING the id directly is the standard pattern used everywhere else in the test suite. Test now matches the real-world insert pattern in api.search + library scan (sort_name mirrors name when no MBID-driven sort hint is available). Other GC tests in this file don't touch artists so they were already fine. --- internal/gc/worker_test.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/internal/gc/worker_test.go b/internal/gc/worker_test.go index eb7ef9ee..5e5f737e 100644 --- a/internal/gc/worker_test.go +++ b/internal/gc/worker_test.go @@ -66,15 +66,13 @@ func TestGcCloseStalePlayEvents_ClosesOnly24hOldRows(t *testing.T) { // Need a track + session to satisfy FKs on play_events. var trackID pgtype.UUID - if err := pool.QueryRow(ctx, ` - INSERT INTO artists (name) VALUES ('A') RETURNING id - `).Scan(new(pgtype.UUID)); err != nil { - t.Fatalf("seed artist row: %v", err) - } - // Get the just-inserted artist id. var artistID pgtype.UUID - if err := pool.QueryRow(ctx, `SELECT id FROM artists WHERE name = 'A'`).Scan(&artistID); err != nil { - t.Fatalf("read artist: %v", err) + // `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, ` -- 2.52.0 From cb2f9a2ea2df3b5ae8938a1dbbc30cb5dcebb099 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 2 Jun 2026 19:04:00 -0400 Subject: [PATCH 11/11] fix(server): GC test seeds tracks with file_size + file_format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second go-round of the same shape of bug: tracks has file_size + file_format NOT NULL (0002_core_library.up.sql) and my GC test seed omitted both. The previous fix only addressed the artists.sort_name column; the tracks INSERT was missing two more. Use plausible stub values — the GC sweep only joins on track_id, none of these columns affect what the test exercises. --- internal/gc/worker_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/gc/worker_test.go b/internal/gc/worker_test.go index 5e5f737e..9cfd24c9 100644 --- a/internal/gc/worker_test.go +++ b/internal/gc/worker_test.go @@ -80,9 +80,14 @@ func TestGcCloseStalePlayEvents_ClosesOnly24hOldRows(t *testing.T) { `, 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) - VALUES ($1, $2, 'T', '/x.mp3', 180000) RETURNING id + 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) } -- 2.52.0