On-device test against Sonos showed SetAVTransportURI returning UPnP
error 714 (IllegalMimeType). Logcat:
POST /api/cast/stream-token -> 200 (token minted)
SetAVTransportURI to http://minstrel.fabledsword.com/...
<-- 500 from Sonos: SoapFaultException SOAP fault 714
The server is behind a TLS-terminating reverse proxy, so r.TLS is
nil and the URL builder emitted http://. Sonos does a HEAD probe to
detect the audio MIME type; against an http:// URL that 301s to
https://, the probe finds no audio body and bails with 714.
The Task 2 code-quality reviewer flagged this exact scenario at the
time. Closing it now: honor X-Forwarded-Proto + X-Forwarded-Host
before falling back to r.TLS + r.Host. Public URL the speaker
fetches now matches the scheme/host the client used to reach the
endpoint.
Debug builds got DebugTree; release builds had no tree planted at
all, so Timber.w / Timber.e calls were dropped silently in
production. That's how the UPnP select diagnostic-prints went
invisible during on-device testing - the released APK had no Timber
output reaching logcat.
Plant a release-only Tree that emits at WARN and above via
android.util.Log.println with the canonical 'Minstrel' tag (or the
caller-supplied tag when present). Keeps DEBUG / INFO traffic out of
production logcat (the chatty stuff is the part we don't want
flooding the buffer) while letting operator-driven adb logcat
sessions still see real failures.
Two silent early returns in selectUpnp were swallowing the most
likely failure modes:
- currentTrack null (nothing playing locally → can't cast a track)
- transportFor() returns null (route disappeared or id mismatch)
On-device verification reported 'tap collapses the sheet but no
audio routes', with logcat empty - one of these was firing without
any signal.
Each early-return now Timber.w's why; the runCatching block adds
Timber.i breadcrumbs at every step (mint token, SetAVTransportURI,
Play, done) so the next failure shows exactly how far we got.
Sonos uses the friendlyName format
'Room - Device Type - RINCON_<UDN>'
The picker was showing it verbatim, so the user saw rows like
'Living Room - Sonos Play:1 Media Renderer - RINCON_5CAAFD79...'
Now strips on the first ' - ' for Sonos manufacturer matches, so the
chip shows just 'Living Room' / 'Kitchen' / etc. Subtitle (manufacturer
+ model) still renders below per the existing sheet design, so the
device-type info isn't lost.
Generic UPnP devices that append a '(192.168.x.x)' IP suffix get that
stripped too via an end-of-string-anchored regex. Empty / blank
friendlyName still falls back to 'Network speaker'.
TestRoutesRegisteredInMount failed because handleGetStream did the
DB lookup (404 on missing track) BEFORE streamAuthOk (401 on
unauth). For an unauth request to a non-existent track, the test
saw 404 and concluded the route wasn't registered when actually it
was - the handler just bailed at the lookup before auth.
Reorder: extract trackID via chi.URLParam, run streamAuthOk on the
raw path id first (the HMAC token is signed over the same id
string so we don't need the resolved row yet), then do the DB
lookup. Test now sees 401 on the unauth probe as it expected.
Also closes a small info-leak: previously a 404/401 differential
let unauth callers probe which track IDs exist. Now both unknown
and known IDs return 401 for unauth requests.
golangci-lint flagged three errcheck:
- stream_token.go: fmt.Fprintf(mac, ...) - hash.Hash never errors
per documented contract, but errcheck wants explicit discard.
Discard via _, _ assignment with a WHY comment.
- config_test.go: os.Unsetenv calls in tests - discard the error
via _ assignment. Test cleanup paths.
Reviewers flagged the Fprintf one during Task 1 quality review but
golangci-lint runs in a separate CI step that wasn't exercised on
the per-task pushes (cancelled by subsequent push concurrency).
go vet caught the test's Mount call missing the trailing []byte
streamSecret arg added by the UPnP slice's Task 2. The test passed nil
for *playlists.Scheduler but didn't pass anything for []byte, so the
arg count was one short.
Added nil for the streamSecret position - the test exercises route
registration only, not the cast-token endpoint, so the secret value
doesn't matter for what this test asserts.
Operator feedback on the prior unification commit (7473e98d):
1. NewForYou should daily-rotate alongside Rediscover and FirstListens.
The 'newest album first regardless of day' intent was the wrong
call - operator wants visible day-over-day movement on every
deterministic mix surface. Spec flipped to dailyRotate: true.
2. Diversity caps (<=2 per album / <=3 per artist) on every mix, not
just the historically-diverse ones. The 2-per-album limit has
helped a lot on the operator's library; extending it to NewForYou
and FirstListens (previously album-coherent / no cap) surfaces
more distinct albums per day. Spec flipped to diversify: true on
all five.
3. Fallback when diversity caps strip the pool below the 100-track
target: finishMix now calls topUpFromRaw, which appends non-capped
tracks from the raw SQL pool (preserving original ranked order +
skipping duplicates) until the target is hit or the pool runs out.
On rich libraries the cap yields >= 100 and top-up never runs; on
thin / album-heavy libraries we ship a partly-diversified 100
instead of a strictly-diversified 40.
Net effect: every deterministic mix now rotates day-over-day, every
mix gets the same diversity treatment (with graceful degradation),
and the producer surface stays a single factory over a spec list.
The five discovery-mix producers (Deep Cuts, Rediscover, New for you,
On this day, First listens) were near-identical boilerplate that
differed only in (a) which SQL query they ran and (b) whether to
diversity-cap the result. Folded into one produceDiscoveryMix(spec)
factory + a per-mix discoveryMixSpec slice. The registry composes the
factory over the spec list so adding a new mix is one struct literal
+ a SQL query, never a new func.
Also fixes the user-reported bug that several mixes 'show the same
content from yesterday'. Audit of the SQL queries:
- Deep Cuts: ORDER BY md5(t.id::text || $2::text) → day-keyed
- On this day: ORDER BY w.c DESC, md5(...) → day-keyed
- Rediscover: ORDER BY tier, c DESC, id → invariant
- New for you: ORDER BY al.created_at DESC, disc, track → invariant
- First listens: ORDER BY tier, al.id, disc, track → invariant
The three invariant ones produced identical content day-over-day. The
unified spec carries a dailyRotate bool: when set, the producer
applies a daily-deterministic offset rotate-left of the candidate
pool BEFORE diversify+truncate. Rotation (not shuffle) preserves
contiguous-block ordering inside each day's slice — matters for First
listens which is album-coherent.
Set on Rediscover + First listens (where same-content-every-day is
clearly a bug). Left off New for you because 'newest album first
regardless of day' is the intended UX for that surface — daily
rotation there would feel wrong.
Daily rotation seed: rand.New(NewSource(int64(userIDHash(userID,
dateStr)))) — same primitive used by For-You's pickHeadAndTail
sampling so behavior is consistent across the system playlist family.
No test file referenced the deleted produceXxx functions directly,
only the registry, so this is a closed refactor.
Two independent bugs surfaced together:
Bug 1: like button on tracks in playlist/album detail screens didn't
reflect actual liked state. LikesRepository.observeLikedTracks() does
a mapNotNull join against trackDao - a liked track whose row isn't in
the local cache yet (e.g. liked via web/notification, cache not
sync'd) gets DROPPED from the returned list. PlaylistDetailViewModel
+ AlbumDetailViewModel both used that as their like-set source, so
those rows showed as not-liked.
Adds LikesRepository.observeLikedTrackIds(): Flow<Set<String>> that
hits the DAO directly via observeLikedIdsOfType - no trackDao join,
no drops. The two ViewModels switch to it. LikedTab continues to use
observeLikedTracks because it needs the full TrackRef to render.
Bug 2: playlist cover art didn't render on the playlist detail
header. Server's derivePlaylistView returns CoverURL as the relative
path "/api/playlists/<id>/cover". PlaylistsRepository's two domain
mappers (CachedPlaylistEntity.toDomain + PlaylistDetailWire
.toPlaylistRef) stored it verbatim - Coil's AsyncImage can't fetch a
relative URL with no base, so the image silently failed.
Wraps the coverPath/coverUrl through resolveServerUrl so the
placeholder.invalid host triggers BaseUrlInterceptor's live-server
rewrite, same idiom every other cover surface (album / artist / track
/ playlist track rows) already uses.
System-playlist 24h refresh investigation pending - need to know how
you verified (server logs, DB state, or client-visible content)
before I can dig into the right layer.
Code-quality review flagged two non-blockers on commit 03cdff54:
1. selectUpnp's runCatching swallowed SOAP / token-mint failures
silently - OkHttp's logger doesn't see them since they happen in
our own deserialize / parse code. Adds Timber.w on the failure
path so operator's on-device Sonos verification can find the
cause in logcat instead of staring at "nothing happened".
2. UpnpDiscoveryController's fetchJob field was assigned but never
read or cancelled. appScope is process-lifetime so the launched
coroutine dies with the process - no explicit cancellation is
needed. Drop the field + the now-unused Job import.
UpnpDiscoveryController - Hilt singleton that owns the SSDP listener,
follows each discovered LOCATION URL to fetch + parse the device
description, projects MediaRenderers into a StateFlow<List<UpnpRoute>>.
OutputPickerController now combines system routes with the UPnP
Flow into a unified RouteSnapshot. select() branches by protocol:
SYSTEM goes through MediaRouter as before; UPNP requests a signed
stream token via POST /api/cast/stream-token then calls
AVTransport.SetAVTransportURI + Play against the discovered device.
Local playback pauses on UPnP selection.
OutputPickerSheet gains a MulticastHintRow shown when no UPnP
devices appear after a 3s grace period - the 'your router may be
blocking multicast' footer hint per the spec.
Closes the UPnP slice spec'd in
docs/superpowers/specs/2026-06-03-android-output-picker-upnp-design.md.
On-device verification pending: pair a Sonos / UPnP speaker, confirm
discovery + selection + playback + the multicast-blocked hint.
SoapClient - minimal SOAP 1.1 envelope builder + POST via the shared
app OkHttpClient. Sets the SOAPACTION + Content-Type headers UPnP
expects, parses the action's Response element as a Map<String,
String>, raises SoapFaultException on a <s:Fault> response with the
UPnP errorCode + errorDescription extracted.
AVTransportClient - thin wrapper over SoapClient pinned to the
AVTransport:1 service. Three actions for v1: SetAVTransportURI /
Play / Stop. Pause + Seek deferred until we have hardware in the
loop to verify per-device quirks.
Three MockWebServer-driven unit tests cover the SOAPACTION header
shape, XML escaping of special chars in arg values, and the fault
response -> SoapFaultException path. kxml2 on the test classpath
(Task 4) makes XmlPullParserFactory resolve on the JVM.
Android's XmlPullParserFactory is a Stub-throwing class in android.jar
on the JVM unit-test classpath; the probe pattern from dc5b8252 was
silently skipping the test suite, which gives false test-coverage
confidence. kxml2 is the same parser implementation Android uses
internally - service-provider lookup picks it up automatically once
on the test classpath.
The probe + Assumptions.assumeTrue skip removed; tests now run
unconditionally.
testImplementation(libs.kxml2) - 2.3.0, MIT-licensed, ~80KB. No
production code change.
SsdpDiscovery - UDP multicast listener on 239.255.255.250:1900.
Passive NOTIFY listen always-on once start() is called; explicit
M-SEARCH M-SEARCH on requestActiveScan() (called when picker sheet
opens). WifiManager.MulticastLock held only while running. Emits
each discovered LOCATION URL on a SharedFlow for downstream
description-fetching.
DeviceDescription - pull-parse the <device> XML returned from a
LOCATION URL, extracting friendlyName / manufacturer / modelName +
AVTransport + RenderingControl service control URLs. Filters out
devices without AVTransport (we can't control them).
Three unit tests cover a Sonos-shaped description, a non-renderer
device that should be dropped, and a minimal description with
missing optional fields.
UpnpRoute - narrow domain model for a discovered UPnP / DLNA
renderer. Carries the AVTransport + RenderingControl control URLs
the SOAP client uses.
CastApi - Retrofit interface for the new POST /api/cast/stream-token
endpoint (UPnP slice 2/6). Returns {token, exp, url} for the
selection path.
OutputRoute.fromUpnpRoute - companion factory that tags the route
with Protocol.UPNP. Subtitle is 'Manufacturer Model' or falls back
to 'Network speaker' when description fields are blank.
CHANGE_WIFI_MULTICAST_STATE manifest permission - install-time on
all API levels, no runtime prompt. Required for SSDP multicast
discovery.
Discovery + SOAP + integration land in follow-up commits.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds the client-facing endpoint that issues a signed stream URL for
the current track. Authenticated via the standard session cookie.
Returns {token, exp, url} where url is a fully-formed stream URL
the client passes verbatim to a UPnP / Sonos device's
AVTransport.SetAVTransportURI call.
expSeconds clamped to [60, 86400]; default 21600 (6h) - long enough
to play through any typical track without re-minting mid-playback.
MINSTREL_STREAM_SECRET is loaded from env var with a per-machine
fallback persisted at <Storage.DataDir>/stream_secret (auto-generated
on first boot via 64 random bytes, base64-url-encoded, 0600). The
file-based fallback is operator-machine-scoped runtime state, not a
user-facing setting - chosen over a DB column to avoid a migration
and keep the secret out of cross-instance restores. Operator can
override at any time via the env var; default path requires zero
config.
Tests cover happy-path token issuance + URL formatting, bad-UUID
rejection, unauthenticated rejection, the expSeconds clamp at all
boundaries, secret env override, auto-gen + file persistence at 0600,
second-boot reuse of the persisted file, and rejection of a malformed
env value.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds SignStreamToken / VerifyStreamToken (HMAC-SHA256 over
trackID|exp) and modifies handleGetStream to accept either the
existing session cookie OR a valid signed token. Stream route
moved out of the authed group so the handler's own auth check
runs and the token bypass is reachable.
Enables Sonos / UPnP speakers to fetch the stream URL without
carrying the user's session cookie - they cannot. The token is
short-lived (max 24h per the design); expiry checked at request
time only, not per-byte, so long tracks play through.
streamSecret field on handlers is nil for now; Task 2 wires the
loader (env var with auto-generated fallback persisted in
app_preferences).
Adds auth.OptionalUser - the permissive sibling of RequireUser
that attaches the user to context when a valid cookie / bearer is
present but does NOT 401 on absence. The stream route is wrapped
with it so the handler can fall through to the token path when
no session is present.
newLibraryRouter (test fixture) gets a synthetic-user middleware
on the stream route so existing media_test tests keep passing
without seeding a real session row - production traffic uses
auth.OptionalUser, the test path uses auth.UserCtxKeyForTest().
Five tests cover round-trip, tampered token rejection, expiry,
wrong-track-ID, and wrong-secret rejection. CI verifies.
Two failures on the slice's final dev tip:
1. OutputPickerController referenced
MediaRouter.CALLBACK_FLAG_PASSIVE_DISCOVERY which doesn't exist
in androidx.mediarouter 1.7.0 - the spec hallucinated it.
Passive discovery is the default behavior when addCallback is
called with no flag argument. Use the 2-arg overload for the
init block and downgradeDiscovery; keep CALLBACK_FLAG_REQUEST_DISCOVERY
for upgradeDiscovery.
2. NowPlayingBody grew to 82 lines after the Task 5 output-picker
wiring (state collection + permission launcher + LaunchedEffect
+ conditional Sheet). Extracted the BLUETOOTH_CONNECT permission
plumbing into rememberBluetoothPermissionState, the Column layout
into NowPlayingContent, and the scrubber+transport pair (which
share the smoothed playback position) into PlaybackControlsBlock.
NowPlayingBody is back to ~34 lines and the new helpers each sit
well under detekt's 60-line LongMethod cap.
Bluetooth slice (5/5). Wires the OutputPickerViewModel + chip +
sheet into NowPlayingScreen.
- Chip renders between BottomActionsRow and ScrubberRow, hidden
via shouldShowChip() when the only route is the built-in speaker
(no useful picker with one option).
- Sheet appears on chip tap; selecting a route or dismissing flips
the ViewModel state and downgrades MediaRouter discovery.
- BLUETOOTH_CONNECT permission requested via the modern
ActivityResultContracts.RequestPermission() pattern on first
sheet open. permissionDenied flag passed through to the sheet so
the 'pair in Settings' hint renders when refused.
Closes the Bluetooth slice spec'd in
docs/superpowers/specs/2026-06-03-android-output-picker-bluetooth-design.md.
On-device verification still pending: pair a Bluetooth speaker,
confirm chip + sheet + select + audio routes; verify wired plug
auto-update + permission-denial hint + long-name truncation.
Spec's edge-case table calls for the BLUETOOTH_CONNECT permission
hint footer to render alongside a Lucide.Settings icon. Task 4
landed the hint text but not the icon. One-line spec fix on top
of commit a319e3f6.
Bluetooth slice (4/5). DeviceChip: Spotify-style current-route
indicator with icon + name + chevron, single-line ellipsis on long
names. iconFor() maps Kind to Lucide icons (Smartphone / Headphones
/ Bluetooth / Cast / Speaker).
OutputPickerSheet: Material 3 ModalBottomSheet. Header 'Output',
rows = icon + name + 2-line subtitle + selection state (CircleCheck
accent for selected, Circle outline otherwise). Tap selects +
dismisses. permissionDenied flag controls a footer hint row when
BLUETOOTH_CONNECT was refused.
NowPlayingScreen wiring lands in the final commit.
Bluetooth slice (3/5). HiltViewModel projecting the controller's
routesState Flow plus a sheetVisible MutableStateFlow that owns
the sheet's open/close state. onChipTapped + onSheetDismissed
forward to the controller's discovery toggle so active MediaRouter
discovery only runs while the sheet is visible (battery cost).
Compose UI + NowPlaying wiring land next.
Bluetooth slice (2/5). Hilt singleton over androidx.mediarouter.
Owns the callback lifecycle (passive at process start, upgrades to
active when the picker sheet opens, reverts on close) and exposes
the route state as a StateFlow<RouteSnapshot> the ViewModel
projects.
Routes are sorted current-first then by Kind (Bluetooth, Wired,
BuiltIn, Other) so the active output is always at the top of the
sheet.
ViewModel + Compose UI follow in next commits.
Bluetooth slice (1/5). Adds the androidx.mediarouter 1.7.0 dep,
declares BLUETOOTH_CONNECT (needed on Android 12+ to enumerate
paired BT devices by name), and lays down the OutputRoute domain
model.
OutputRoute decouples the picker UI from MediaRouter.RouteInfo
(framework class, can't be constructed in JVM tests - same
constraint we hit with LikeMediaCallback). The Protocol enum
includes UPNP/CAST/SONOS placeholders so the next slice slots in
without a data-model rename - see
docs/superpowers/specs/2026-06-03-android-output-picker-upnp-scope.md
for the deferred work.
Controller + ViewModel + Compose UI land in follow-up commits.
Crash on cold boot: ResumeController.restore is suspend, lands on
Dispatchers.Default after awaitReady() unblocks (drift #562), and
calls PlayerController.setQueue which calls MediaController.setMediaItems
— MediaController enforces application-thread access and throws
IllegalStateException 'method is called from a wrong thread'.
Drift #562 added awaitReady() to fix the race where setQueue
early-returned on null controller and silently dropped the persisted
queue. That fix exposed the next bug down the stack: the threading
violation that was previously masked by the early-return.
setQueue now posts the MediaController calls to the controller's
applicationLooper if we're not already on it. UI callers (already
Main) run inline with no re-dispatch latency. ResumeController's
cold-boot path lands on the right thread.
Discovered on-device 2026-06-03 during like-button verification on
the Pixel 6 Pro emulator — crash log at PlayerController.kt:190.
Line 264 already null-checks playlist.systemVariant in the if
condition. PlaylistRef is a data class with a val backing field,
so the smart cast narrows it to String inside the branch — the
!! on line 265 was a no-op the Kotlin compiler was warning about.
MinstrelPlayerService now injects LikesRepository, attaches the new
LikeMediaCallback, sets an initial unfilled CommandButton via
setMediaButtonPreferences, and launches a service-scoped job that
rebuilds the preferences list when the current track or its
server-side liked state changes.
flatMapLatest on (currentMediaItem x observeIsLiked) means the icon
mirrors cross-device likes (web tap flips the notification heart
within EventsStream propagation) and never leaks Flows across track
transitions. Initial emission on subscription guarantees the icon is
correct on the first frame the controller renders.
onDestroy now cancels the service scope before releasing the session
so the like-state job can't touch a released MediaSession.
Closes the Media3 like-button work spec'd in
docs/superpowers/specs/2026-06-02-android-media3-like-button-design.md.
On-device verification still pending: phone notification, lock
screen, Pixel Watch, Android Auto, offline replay, cross-device.
The unit tests called Media3's SessionCommand(String, Bundle)
constructor, which checkNotNulls the Bundle. JVM unit tests have
no real Android — Bundle.EMPTY is a static field initialized via
the stub jar to null. isReturnDefaultValues=true escapes the
ExceptionInInitializerError but leaves Bundle.EMPTY as null, so
SessionCommand still NPEs on construction. The real fix is
Robolectric, which is disproportionate infrastructure for one
test file (pulls in JUnit 4 ceremony for a JUnit 5 project + a
heavy dep + first-run SDK download flake risk on this CI).
Verification gate for the like button is operator on-device check
per feedback_definition_of_done. The Task 2 wiring lands next,
then we verify the heart appears on the phone notification, lock
screen, and Pixel Watch end-to-end.
Unit tests touching Android framework statics (Bundle.EMPTY,
android.os.Bundle constructor in MediaItem/SessionCommand
construction) failed with NPE/ExceptionInInitializerError because
JVM unit tests run against android.jar's stub classes whose methods
throw "Method ... not mocked" by default. Enable
isReturnDefaultValues so stub methods return defaults — Bundle.EMPTY
ends up null and is fine because we just thread it through
SessionCommand without inspecting it.
Fixes LikeMediaCallbackTest's 5 failures on run #311. Lightweight —
no Robolectric, no androidTest. The first JVM-side test file in the
project to touch Android framework classes.
detekt: onCustomCommand had 3 returns (unsupported / no-mediaItem /
success), ReturnCount cap is 2. Pull the toggle path into a private
launchToggleForCurrent helper so onCustomCommand is a single
return (if/else picks the result code, one Future wrap) and the
helper has at most 2 returns.
New MediaSession.Callback that grants CMD_TOGGLE_LIKE in onConnect
(Media3 issue #2679 guard) and routes onCustomCommand through
LikesRepository.toggleLike so notification/lock-screen/Pixel-Watch
taps inherit the offline-resilient MutationQueue path.
Unit tests cover the onConnect grant, current-state inversion in
both directions, no-op when there is no current MediaItem, and
rejection of unknown custom actions.
MinstrelPlayerService wiring lands in a follow-up commit.
User report: the round dot didn't read as vertically centered on
the 4dp track even though geometrically it was (M3's SliderLayout
centers the track slot within the thumb's height). A small circle
on a thin horizontal bar is a known perceptual offset — the eye
expects the bar to bisect the circle, but the circle's mass extends
above and below in equal amounts the brain reads as a lift.
Swap the 14dp circle for a 4dp x 18dp vertical pill (CircleShape
on a non-square Box renders as a stadium). Same width as the track,
clearly taller — the bar visibly passes through the pill's
horizontal axis with no ambiguity. Also aligns with M3 expressive's
new vertical-handle slider direction.
Updates the ScrubTrack docstring that still referenced the prior
14dp-on-4dp pairing.
Drift #568/#569 scoped AuthCookieInterceptor to PLACEHOLDER_HOST so
the shared OkHttp client wouldn't leak the session cookie to external
image fetches (Coil → musicbrainz, coverartarchive, Lidarr). The fix
was correct but assumed AuthCookieInterceptor would see the original
placeholder.invalid URL — production NetworkModule had BaseUrlInterceptor
running FIRST, so by the time auth's intercept() ran the host was
already rewritten to the real Minstrel server and the placeholder
check failed on every request.
Symptom on v2026.06.02: fresh install login appears to succeed but
no cookie is captured from Set-Cookie and no cookie is attached to
subsequent requests, so the user stays at the Welcome screen.
AuthCookieInterceptorTest already chains the interceptors in the
correct order, which is why the regression went undetected — only
production was wrong.
Fix: swap to (auth, baseUrl, logging). Auth now sees
placeholder.invalid, attaches/captures the cookie, then BaseUrl
rewrites the host for transport.
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.
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.
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).
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.
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.
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.
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.
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.
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.
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.
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.
The APK was shipping with a hardcoded versionName="0.1.0-native"
and versionCode=1 — so the About card never reflected the actual
release, and two same-day re-cuts of the per-day mutable tag
v2026.06.02 looked identical to the update-banner comparator.
Operator wants the iteration restored on the APK side (the docker
tag stays plain per the earlier intentional change).
Scheme:
- Per release: versionName = "${tag}.${commit_count}", e.g.
"2026.06.02.142", where commit_count = `git rev-list --count HEAD`.
Monotonic across the project lifetime, deterministic, no manual
counter to maintain.
- versionCode = commit_count. Monotonic, fits in Int forever (we're
not hitting 2.1B commits).
- Local / debug / dev builds fall back to versionName="dev" /
versionCode=1 so the About card reads honestly.
build.gradle.kts:
- defaultConfig reads MINSTREL_VERSION_NAME / MINSTREL_VERSION_CODE
Gradle properties via project.findProperty with the dev fallbacks.
.gitea/workflows/release.yml:
- android-release: checkout with fetch-depth: 0 (the default shallow
clone would return 1 for `git rev-list --count HEAD`); new
Compute release version step exports name + code as step outputs;
assembleRelease passes them via -P; new job-level outputs propagate
them to the downstream image-release job.
- image-release: Stage bundled APK + version sidecar pulls the
computed version_name from needs.android-release.outputs and
writes it into client/minstrel.apk.version, so the server's
/api/client/version reports the exact string baked into the APK.
Without this the sidecar would say "v2026.06.02" while the
installed APK has "2026.06.02.142" — isVersionNewer would call
the bundled APK older and the update banner would thrash.
The existing isVersionNewer comparator already handles the
4-component shape ("2026.06.02.142" > "2026.06.02.141"), so no
client-side logic changes are needed.
The kdoc on PlaybackErrorReportRequest mentioned the existing
/api/plays/* endpoint. Kotlin's lexer treats /* inside a /** ... */
kdoc as a NESTED comment opener, which then swallows the outer
*/ — so the entire PlaybackErrorReportRequest data class
disappeared from the symbol table and the four call sites in
PlaybackErrorsApi.kt / MutationReplayer.kt / PlaybackErrorRepository.kt
all reported "Unresolved reference".
This is the trap recorded in the project's KSP-could-not-be-
resolved memory; mark it again. Fix is mechanical: rewrite the
prose as `/api/plays/...` so no /* sequence appears inside a
block comment.
Three rule trips from the playback-errors + scrubber commits:
PlayerController.startRadio (4 returns → 2): extract the mid-queue
append branch into appendRadioToQueue(). startRadio just does the
guard checks and dispatches; the helper handles the cursor trim +
addMediaItems. Behavior identical.
PlayerController.onPlaybackStateChanged (4 returns → 2): extract
the duration check + zero-duration error emission + skip logic
into handleZeroDurationIfNeeded(). The listener stays compact (one
return for non-READY, one for repeat-evaluation guard); the helper
owns the failure path.
NowPlayingScreen.ScrubberRow (63 lines → ~50): extract the custom
track Box block into a ScrubTrack(fraction, accent) composable.
The Slider's `track` lambda becomes a one-line call. Pixel output
is identical.
Surfaces client-reported playback failures from /api/admin/playback-errors
in a new admin tab. Tabs: Unresolved (default) / Resolved. Each row
shows track + artist + album, error kind badge, who hit it, when,
optional client-supplied detail, and the absolute file path so the
operator can grep the library mount without leaving the page.
Per-row actions (RowActionsMenu):
- Resolve (primary) — modal with Fixed / Ignored dropdown for the
"no further action taken" cases.
- Copy — JSON payload to clipboard with track_id / file_path / kind
/ detail / reporter / client_id / occurred_at. Matches the
operator's "logs with a copy-out function" ask.
- Delete file (danger, modal-confirm) — uses the existing
/api/admin/quarantine/{track_id}/delete-file endpoint AND
auto-stamps resolution='deleted' so a single click closes both
the file and the inbox row.
Deferred to a follow-up: Hide (the existing quarantine flow is
per-user-flag, not a true library-hide), and Re-request via Lidarr
(needs album MBID join — not in the current ListAdminPlaybackErrors
projection).
Also: admin tab list grows from five to six; AdminTabs.test.ts
updated.
Operator hit a track that loaded with zero duration; player just sat
on it. Two things needed: skip the dead track immediately, and tell
the server so the admin inbox can surface the bad file.
PlayerController:
- Player.Listener.onPlaybackStateChanged(STATE_READY) now checks
duration. If it's <= 0 or C.TIME_UNSET, fires a PlaybackErrorEvent
with kind="zero_duration" and calls seekToNextMediaItem (or stop
if it was the last item). Per-item evaluation guard keeps repeat
STATE_READY events (post-seek, post-resume) from re-firing.
- onPlayerError now also surfaces a PlaybackErrorEvent with
kind="load_failed" + the Media3 exception message as detail.
- playbackErrorEvents flow changes from Flow<String> (title only) to
Flow<PlaybackErrorEvent> (track_id + kind + title + detail) so
downstream consumers can both surface a snackbar AND POST to the
admin inbox without duplicating event emission.
PlaybackErrorRepository (new):
- Wraps POST /api/playback-errors with the offline-first MutationQueue
fallback per the standing rule for server writes.
- Reuses AuthStore.clientId for the client_id field — same UUID-per-
install identifier the play-events reporter sends, so support can
correlate playback errors with surrounding plays.
PlaybackErrorReporter:
- Consumes the new richer event shape. Fires the server report per
event (no debounce — the admin inbox should capture every report,
not a coalesced summary). Continues to debounce the user-facing
snackbar in the 2s window so a burst doesn't spam toasts.
MutationQueue / MutationReplayer:
- Adds PLAYBACK_ERROR_REPORT kind + PlaybackErrorReportPayload +
enqueuePlaybackErrorReport entry point + replayer dispatch case
hitting the new PlaybackErrorsApi.
Web admin inbox + UI is the next commit.
bvandeusen