Drift audit 2026-06-02 — 26 findings shipped #75

Merged

bvandeusen merged 11 commits from dev into main 2026-06-02 19:21:53 -04:00

Conversation 0 Commits 11 Files Changed 29

+1104 -88

bvandeusen commented 2026-06-02 19:20:43 -04:00

Owner

Copy Link

Copy Source

Multi-system drift audit from 2026-06-02: 14 auditors (12 subsystem + 2 cross-cutting) fanned out across the codebase looking for the 10-pattern drift taxonomy. 26 candidate findings surfaced; 24 confirmed by adversarial verify and shipped, 1 cancelled as a duplicate, 1 doc-only correction (implementation tracked separately).

Scribe parent task: project 12 / task 552. All 26 child tasks closed.

Batches

1. 47d2f611 — batch 1 (6 mechanical wins): web Tailwind token (bg-action-danger → bg-action-destructive, #553); TS type for play_started source (#555); coverage rollup whitelist + test extended to deezer/lastfm (#556 #557); /forgot-password + /reset-password/[token] added to PUBLIC_ROUTES (#558); scanner extensions aligned to media handler (.opus/.aac/.wav, #571).
2. fb3116d6 — batch 2 (5 patterned fixes): playback-error channel CONFLATED → BUFFERED so burst-counting works (#561); system-playlist rotation whitelist gains 6 missing variants (#563); Android source format aligned with server (bare variant, not playlist:<variant>, #564); AuthCookieInterceptor scoped to placeholder host (#568 #569).
3. b19c6217 — batch 3a (offline correctness): LikesRepository.refreshIds atomic replaceAllForUser so cross-device unlike clears (#570); RequestsViewModel.cancel only refresh()s on Synced outcome (#577).
4. 5014f754 — batch 3b (cold-boot resume): PlayerController.setQueue gains autoplay opt-out so ResumeController doesn't auto-start playback on cold launch (#560); awaitReady() suspend lets ResumeController wait for MediaController bind (#562).
5. b970b873 — batch 4: server /api/me returns full profile shape (display_name + email) so Android Settings → Profile stops wiping users' stored values (#578).
6. 413d7297 — batch 5 (web): +page.server.ts → +page.ts so the /library and /playlists redirects actually run in the static SPA build (#559); radio auto-refresh caps exclude= at 100 ids to avoid 414 URI Too Long (#554).
7. bda0896d — batch 6: delete.go docstring made honest about the missing orphan-row reconcile (#572 — implementation kept as a tracked follow-up).
8. 258bc1f7 — batch 7 (GC worker): new internal/gc package with one Worker running 5 idempotent sweeps on a 1h tick: orphan play_events close (#566), play_sessions ended_at writer (#565), expired scrobble_queue retention (#567), system_playlist_runs stuck-row reaper (#574), expired password_resets (#575).
9. dbcadf0f — batch 8: LikesRepository uses real user UUID from AuthController.currentUser instead of LOCAL_USER_ID, clears outgoing user's cached_likes on user-switch (#576).

Plus 305d4780 + cb2f9a2e — two test-seed fixes for NOT NULL columns the GC integration test was missing (artists.sort_name, then tracks.file_size + file_format). Both caught by Go integration CI on dev before this PR.

Test plan

• CI: dev test-go (short + integration) green on cb2f9a2e
• CI: dev Android Build+lint+test green on dbcadf0f (latest commit that touched Kotlin)
• CI: dev test-web Vitest green on 413d7297 (latest commit that touched .ts/.svelte)
• On device: Android Settings → Profile shows stored display_name/email (regression for #578)
• On web: signed-out user can reach /forgot-password and /reset-password/[token] without redirect (#558)
• On Android: tap Cancel on a request while offline — UI stays calm, no Error state (#577)
• On Android cold start with paused playback: opens to paused queue, doesn't auto-play (#560)
• Operator-visible: admin Coverage dashboard reports correct totals when deezer/lastfm sources are in use (#556)

Multi-system drift audit from 2026-06-02: 14 auditors (12 subsystem + 2 cross-cutting) fanned out across the codebase looking for the 10-pattern drift taxonomy. 26 candidate findings surfaced; 24 confirmed by adversarial verify and shipped, 1 cancelled as a duplicate, 1 doc-only correction (implementation tracked separately). Scribe parent task: project 12 / task 552. All 26 child tasks closed. ## Batches 1. **`47d2f611` — batch 1 (6 mechanical wins)**: web Tailwind token (`bg-action-danger` → `bg-action-destructive`, #553); TS type for play_started `source` (#555); coverage rollup whitelist + test extended to deezer/lastfm (#556 #557); /forgot-password + /reset-password/[token] added to PUBLIC_ROUTES (#558); scanner extensions aligned to media handler (.opus/.aac/.wav, #571). 2. **`fb3116d6` — batch 2 (5 patterned fixes)**: playback-error channel CONFLATED → BUFFERED so burst-counting works (#561); system-playlist rotation whitelist gains 6 missing variants (#563); Android source format aligned with server (bare variant, not `playlist:<variant>`, #564); AuthCookieInterceptor scoped to placeholder host (#568 #569). 3. **`b19c6217` — batch 3a (offline correctness)**: LikesRepository.refreshIds atomic replaceAllForUser so cross-device unlike clears (#570); RequestsViewModel.cancel only refresh()s on Synced outcome (#577). 4. **`5014f754` — batch 3b (cold-boot resume)**: PlayerController.setQueue gains `autoplay` opt-out so ResumeController doesn't auto-start playback on cold launch (#560); awaitReady() suspend lets ResumeController wait for MediaController bind (#562). 5. **`b970b873` — batch 4**: server /api/me returns full profile shape (display_name + email) so Android Settings → Profile stops wiping users' stored values (#578). 6. **`413d7297` — batch 5 (web)**: +page.server.ts → +page.ts so the /library and /playlists redirects actually run in the static SPA build (#559); radio auto-refresh caps `exclude=` at 100 ids to avoid 414 URI Too Long (#554). 7. **`bda0896d` — batch 6**: delete.go docstring made honest about the missing orphan-row reconcile (#572 — implementation kept as a tracked follow-up). 8. **`258bc1f7` — batch 7 (GC worker)**: new `internal/gc` package with one Worker running 5 idempotent sweeps on a 1h tick: orphan play_events close (#566), play_sessions ended_at writer (#565), expired scrobble_queue retention (#567), system_playlist_runs stuck-row reaper (#574), expired password_resets (#575). 9. **`dbcadf0f` — batch 8**: LikesRepository uses real user UUID from AuthController.currentUser instead of LOCAL_USER_ID, clears outgoing user's cached_likes on user-switch (#576). Plus `305d4780` + `cb2f9a2e` — two test-seed fixes for NOT NULL columns the GC integration test was missing (artists.sort_name, then tracks.file_size + file_format). Both caught by Go integration CI on dev before this PR. ## Test plan - [x] CI: dev test-go (short + integration) green on `cb2f9a2e` - [x] CI: dev Android Build+lint+test green on `dbcadf0f` (latest commit that touched Kotlin) - [x] CI: dev test-web Vitest green on `413d7297` (latest commit that touched .ts/.svelte) - [ ] On device: Android Settings → Profile shows stored display_name/email (regression for #578) - [ ] On web: signed-out user can reach /forgot-password and /reset-password/[token] without redirect (#558) - [ ] On Android: tap Cancel on a request while offline — UI stays calm, no Error state (#577) - [ ] On Android cold start with paused playback: opens to paused queue, doesn't auto-play (#560) - [ ] Operator-visible: admin Coverage dashboard reports correct totals when deezer/lastfm sources are in use (#556)

bvandeusen added 11 commits 2026-06-02 19:20:43 -04:00

fix: drift audit batch 1 — six small mechanical wins ... 47d2f61161

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.

fix: drift audit batch 2 — patterned fixes mirroring prior work ... fb3116d640

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 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.

fix: drift audit batch 3a — Android offline correctness ... b19c621743

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.

fix: drift audit batch 3b — cold-boot resume correctness ... 5014f7548e

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.

fix(server): drift #578 — /api/me returns profile shape with display_name + email ... b970b87343

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.

fix(web): drift audit batch 5 — SSR redirect + radio exclude cap ... 413d729711

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.

docs(server): drift #572 — delete.go honest about missing reconcile ... bda0896d82

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.

feat(server): drift audit batch 7 — periodic GC worker for 5 lifecycle gaps ... 258bc1f75c

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.

fix(android): drift #576 — LikesRepository uses real userId, clears on switch ... dbcadf0f93

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).

fix(server): TestGcCloseStalePlayEvents seeds artist with sort_name ... 305d4780ac

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.

fix(server): GC test seeds tracks with file_size + file_format ... cb2f9a2ea2

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 merged commit 46dcd38fd8 into main 2026-06-02 19:21:53 -04:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

deps

Dependency update (Renovate)

deps:major

Major dependency bump — held for manual review

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

bvandeusen

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: bvandeusen/minstrel#75