Drift audit 2026-06-02 — 26 findings shipped #75
Merged
bvandeusen
merged 11 commits from 2026-06-02 19:21:53 -04:00
dev into main
11 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 bvandeusen
|
cb2f9a2ea2 |
fix(server): GC test seeds tracks with file_size + file_format
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. |
||
|
bvandeusen
|
305d4780ac |
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. |
||
|
bvandeusen
|
dbcadf0f93 |
fix(android): drift #576 — LikesRepository uses real userId, clears on switch
android / Build + lint + test (push) Successful in 4m1s
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). |
||
|
bvandeusen
|
258bc1f75c |
feat(server): drift audit batch 7 — periodic GC worker for 5 lifecycle gaps
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. |
||
|
bvandeusen
|
bda0896d82 |
docs(server): drift #572 — delete.go honest about missing reconcile
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. |
||
|
bvandeusen
|
413d729711 |
fix(web): drift audit batch 5 — SSR redirect + radio exclude cap
test-web / test (push) Successful in 33s
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. |
||
|
bvandeusen
|
b970b87343 |
fix(server): drift #578 — /api/me returns profile shape with display_name + email
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. |
||
|
bvandeusen
|
5014f7548e |
fix: drift audit batch 3b — cold-boot resume correctness
android / Build + lint + test (push) Successful in 3m49s
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. |
||
|
bvandeusen
|
b19c621743 |
fix: drift audit batch 3a — Android offline correctness
android / Build + lint + test (push) Has been cancelled
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. |
||
|
bvandeusen
|
fb3116d640 |
fix: drift audit batch 2 — patterned fixes mirroring prior work
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:<variant>"
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:<id> tag when the playlist is a refreshable system mix.
User playlists keep playlist:<id> (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
|
||
|
bvandeusen
|
47d2f61161 |
fix: drift audit batch 1 — six small mechanical wins
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/<token> — 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. |