From 2be07ef271c752afb6c180e6debe86591903dfbe Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 3 Jul 2026 08:47:01 -0400 Subject: [PATCH] fix(mixes): close three intent gaps found in the system-playlists audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three discovery-mix defects from the intent audit (Scribe note #1254), all sharing the same root pattern — skips treated as non-events: - Deep Cuts (#1257): eligibility counted only unskipped plays, so a track skipped twice with zero completed listens read as "barely heard" and kept being re-offered. Tracks with >=2 skips no longer qualify; a single accidental skip doesn't banish. - Rediscover (#1258): a skip on a rediscover-sourced play — the user explicitly declining the resurfacing invitation — changed nothing, so declined tracks re-qualified the next day forever. Such tracks now sit out 90 days. - On This Day (#1256): day-of-year distance used plain ABS, so Dec 28 vs Jan 3 read as 359 days apart and the window silently gutted itself for ~3 weeks around every New Year. Now circular (LEAST(d, 365-d)), anchored on the build-date parameter instead of now() so it's testable and consistent with the mix's daily determinism. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01TsF3cNoKrqCYsU78cXC8U6 --- internal/db/dbq/system_mixes.sql.go | 38 ++++- internal/db/queries/system_mixes.sql | 38 ++++- internal/playlists/system_mixes_db_test.go | 179 +++++++++++++++++++++ 3 files changed, 253 insertions(+), 2 deletions(-) create mode 100644 internal/playlists/system_mixes_db_test.go diff --git a/internal/db/dbq/system_mixes.sql.go b/internal/db/dbq/system_mixes.sql.go index ad304865..f43be18f 100644 --- a/internal/db/dbq/system_mixes.sql.go +++ b/internal/db/dbq/system_mixes.sql.go @@ -27,12 +27,20 @@ play_counts AS ( FROM play_events WHERE user_id = $1 AND was_skipped = false GROUP BY track_id +), +skip_counts AS ( + SELECT track_id, COUNT(*) AS c + FROM play_events + WHERE user_id = $1 AND was_skipped = true + GROUP BY track_id ) SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN affinity_artists aa ON aa.artist_id = t.artist_id LEFT JOIN play_counts pc ON pc.track_id = t.id + LEFT JOIN skip_counts sc ON sc.track_id = t.id WHERE COALESCE(pc.c, 0) <= 2 + AND COALESCE(sc.c, 0) < 2 AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id @@ -60,6 +68,11 @@ type ListDeepCutsTracksRow struct { // gives per-play variety on top. // #419 Deep Cuts: low-play tracks (<=2 plays) from artists the user // has liked OR played heavily (>=5 non-skip plays across the artist). +// Tracks the user has skipped twice or more don't qualify (#1257): +// eligibility used to count only unskipped plays, so the most actively +// rejected tracks read as "barely heard" and kept being re-offered. +// Threshold 2 so a single accidental skip doesn't banish a track; +// passive-signal only (no dislike UI, rule #101). // $1 user_id, $2 date string. func (q *Queries) ListDeepCutsTracks(ctx context.Context, arg ListDeepCutsTracksParams) ([]ListDeepCutsTracksRow, error) { rows, err := q.db.Query(ctx, listDeepCutsTracks, arg.UserID, arg.Column2) @@ -203,7 +216,10 @@ WITH windowed AS ( FROM play_events WHERE user_id = $1 AND was_skipped = false AND started_at < now() - interval '30 days' - AND ABS(EXTRACT(DOY FROM started_at) - EXTRACT(DOY FROM now())) <= 10 + AND LEAST( + ABS(EXTRACT(DOY FROM started_at) - EXTRACT(DOY FROM $2::date)), + 365 - ABS(EXTRACT(DOY FROM started_at) - EXTRACT(DOY FROM $2::date)) + ) <= 10 GROUP BY track_id ) SELECT t.id, t.album_id, t.artist_id @@ -235,6 +251,13 @@ type ListOnThisDayTracksRow struct { // and window ±7→±10 so it surfaces on a months-old library instead // of needing a full year of history; still skips cleanly (no rows → // no playlist) when there's no qualifying history yet. +// Day-of-year distance is circular (#1256): plain ABS made Dec 28 vs +// Jan 3 read as 359 days apart, silently gutting the window for ~3 +// weeks around every New Year — precisely when holiday nostalgia is +// the point. LEAST(d, 365-d) wraps the boundary; leap-year drift of +// ±1 is absorbed by the ±10 window. The build date ($2, already here +// for the md5 rotation) anchors "today" instead of now() so the +// window is testable and consistent with the mix's daily determinism. // $1 user_id, $2 date string. func (q *Queries) ListOnThisDayTracks(ctx context.Context, arg ListOnThisDayTracksParams) ([]ListOnThisDayTracksRow, error) { rows, err := q.db.Query(ctx, listOnThisDayTracks, arg.UserID, arg.Column2) @@ -272,6 +295,19 @@ SELECT t.id, t.album_id, t.artist_id SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id ) + -- Declined-resurfacing cooldown (#1258): a skip on a + -- rediscover-sourced play is the user explicitly saying "I've moved + -- on" to the exact invitation this mix extends — such tracks sit + -- out ~90 days instead of re-qualifying the next day. Passive + -- signal only (rule #101). + AND NOT EXISTS ( + SELECT 1 FROM play_events rp + WHERE rp.user_id = $1 + AND rp.track_id = t.id + AND rp.source = 'rediscover' + AND rp.was_skipped = true + AND rp.started_at > now() - interval '90 days' + ) ORDER BY (s.last_at <= now() - interval '6 months') DESC, (s.c >= 5) DESC, s.c DESC, t.id diff --git a/internal/db/queries/system_mixes.sql b/internal/db/queries/system_mixes.sql index 13d3155b..dd106b47 100644 --- a/internal/db/queries/system_mixes.sql +++ b/internal/db/queries/system_mixes.sql @@ -8,6 +8,11 @@ -- name: ListDeepCutsTracks :many -- #419 Deep Cuts: low-play tracks (<=2 plays) from artists the user -- has liked OR played heavily (>=5 non-skip plays across the artist). +-- Tracks the user has skipped twice or more don't qualify (#1257): +-- eligibility used to count only unskipped plays, so the most actively +-- rejected tracks read as "barely heard" and kept being re-offered. +-- Threshold 2 so a single accidental skip doesn't banish a track; +-- passive-signal only (no dislike UI, rule #101). -- $1 user_id, $2 date string. WITH affinity_artists AS ( SELECT artist_id FROM general_likes_artists WHERE user_id = $1 @@ -23,12 +28,20 @@ play_counts AS ( FROM play_events WHERE user_id = $1 AND was_skipped = false GROUP BY track_id +), +skip_counts AS ( + SELECT track_id, COUNT(*) AS c + FROM play_events + WHERE user_id = $1 AND was_skipped = true + GROUP BY track_id ) SELECT t.id, t.album_id, t.artist_id FROM tracks t JOIN affinity_artists aa ON aa.artist_id = t.artist_id LEFT JOIN play_counts pc ON pc.track_id = t.id + LEFT JOIN skip_counts sc ON sc.track_id = t.id WHERE COALESCE(pc.c, 0) <= 2 + AND COALESCE(sc.c, 0) < 2 AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id @@ -63,6 +76,19 @@ SELECT t.id, t.album_id, t.artist_id SELECT 1 FROM lidarr_quarantine q WHERE q.user_id = $1 AND q.track_id = t.id ) + -- Declined-resurfacing cooldown (#1258): a skip on a + -- rediscover-sourced play is the user explicitly saying "I've moved + -- on" to the exact invitation this mix extends — such tracks sit + -- out ~90 days instead of re-qualifying the next day. Passive + -- signal only (rule #101). + AND NOT EXISTS ( + SELECT 1 FROM play_events rp + WHERE rp.user_id = $1 + AND rp.track_id = t.id + AND rp.source = 'rediscover' + AND rp.was_skipped = true + AND rp.started_at > now() - interval '90 days' + ) ORDER BY (s.last_at <= now() - interval '6 months') DESC, (s.c >= 5) DESC, s.c DESC, t.id @@ -103,13 +129,23 @@ SELECT t.id, t.album_id, t.artist_id -- and window ±7→±10 so it surfaces on a months-old library instead -- of needing a full year of history; still skips cleanly (no rows → -- no playlist) when there's no qualifying history yet. +-- Day-of-year distance is circular (#1256): plain ABS made Dec 28 vs +-- Jan 3 read as 359 days apart, silently gutting the window for ~3 +-- weeks around every New Year — precisely when holiday nostalgia is +-- the point. LEAST(d, 365-d) wraps the boundary; leap-year drift of +-- ±1 is absorbed by the ±10 window. The build date ($2, already here +-- for the md5 rotation) anchors "today" instead of now() so the +-- window is testable and consistent with the mix's daily determinism. -- $1 user_id, $2 date string. WITH windowed AS ( SELECT track_id, COUNT(*) AS c FROM play_events WHERE user_id = $1 AND was_skipped = false AND started_at < now() - interval '30 days' - AND ABS(EXTRACT(DOY FROM started_at) - EXTRACT(DOY FROM now())) <= 10 + AND LEAST( + ABS(EXTRACT(DOY FROM started_at) - EXTRACT(DOY FROM $2::date)), + 365 - ABS(EXTRACT(DOY FROM started_at) - EXTRACT(DOY FROM $2::date)) + ) <= 10 GROUP BY track_id ) SELECT t.id, t.album_id, t.artist_id diff --git a/internal/playlists/system_mixes_db_test.go b/internal/playlists/system_mixes_db_test.go new file mode 100644 index 00000000..ae6d6c0d --- /dev/null +++ b/internal/playlists/system_mixes_db_test.go @@ -0,0 +1,179 @@ +package playlists_test + +import ( + "context" + "fmt" + "path/filepath" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// seedTrackForArtist creates another track under an existing artist + +// album pair. seedTrack's mbid-less upsert never dedupes artists, and +// Deep Cuts' affinity rule needs several tracks sharing one artist_id. +func seedTrackForArtist(t *testing.T, pool *pgxpool.Pool, title string, albumID, artistID pgtype.UUID) dbq.Track { + t.Helper() + track, err := dbq.New(pool).UpsertTrack(context.Background(), dbq.UpsertTrackParams{ + Title: title, AlbumID: albumID, ArtistID: artistID, + DurationMs: 1000, + FilePath: filepath.Join(t.TempDir(), fmt.Sprintf("%s.mp3", title)), + FileSize: 100, FileFormat: "mp3", + }) + if err != nil { + t.Fatalf("seed track for artist: %v", err) + } + return track +} + +// seedSourcedPlayEvent is seedPlayEvent plus a source tag — needed by +// the Rediscover cooldown, which keys off rediscover-sourced skips. +func seedSourcedPlayEvent(t *testing.T, pool *pgxpool.Pool, userID, trackID pgtype.UUID, startedAt time.Time, wasSkipped bool, source string) { + t.Helper() + _, err := pool.Exec(context.Background(), ` + WITH s AS ( + INSERT INTO play_sessions (user_id, started_at, last_event_at) + VALUES ($1, $3, $3) + RETURNING id + ) + INSERT INTO play_events (user_id, track_id, session_id, started_at, was_skipped, source) + SELECT $1, $2, s.id, $3, $4, $5 FROM s + `, userID, trackID, startedAt, wasSkipped, source) + if err != nil { + t.Fatalf("seed sourced play_event: %v", err) + } +} + +// idSet collapses candidate rows into a lookup by track id. +func idSet[T any](rows []T, id func(T) pgtype.UUID) map[pgtype.UUID]bool { + out := make(map[pgtype.UUID]bool, len(rows)) + for _, r := range rows { + out[id(r)] = true + } + return out +} + +func TestListDeepCutsTracks_SkippedTracksExcluded(t *testing.T) { + // #1257: eligibility used to count only unskipped plays, so a track + // skipped repeatedly (and never finished) read as "barely heard". + pool := newPool(t) + q := dbq.New(pool) + u := seedUser(t, pool, "dc1") + now := time.Now().UTC() + + // Affinity artist: 5 non-skip plays on the heavy track. + heavy := seedTrack(t, pool, "dc1-heavy", "dc1-artist") + for p := 0; p < 5; p++ { + seedPlayEvent(t, pool, u.ID, heavy.ID, now.Add(-time.Duration(p+1)*time.Hour), false) + } + // Actively rejected: two skips, zero completed plays. + rejected := seedTrackForArtist(t, pool, "dc1-rejected", heavy.AlbumID, heavy.ArtistID) + seedPlayEvent(t, pool, u.ID, rejected.ID, now.Add(-2*time.Hour), true) + seedPlayEvent(t, pool, u.ID, rejected.ID, now.Add(-3*time.Hour), true) + // Genuinely unexplored. + unheard := seedTrackForArtist(t, pool, "dc1-unheard", heavy.AlbumID, heavy.ArtistID) + // One accidental skip stays below the threshold. + oneSkip := seedTrackForArtist(t, pool, "dc1-oneskip", heavy.AlbumID, heavy.ArtistID) + seedPlayEvent(t, pool, u.ID, oneSkip.ID, now.Add(-4*time.Hour), true) + + rows, err := q.ListDeepCutsTracks(context.Background(), dbq.ListDeepCutsTracksParams{ + UserID: u.ID, Column2: "2026-07-03", + }) + if err != nil { + t.Fatalf("ListDeepCutsTracks: %v", err) + } + got := idSet(rows, func(r dbq.ListDeepCutsTracksRow) pgtype.UUID { return r.ID }) + if got[rejected.ID] { + t.Error("twice-skipped track qualified as a deep cut; skips must disqualify") + } + if !got[unheard.ID] { + t.Error("unexplored track missing from deep cuts") + } + if !got[oneSkip.ID] { + t.Error("single accidental skip must not banish a track") + } + if got[heavy.ID] { + t.Error("heavily-played track must not be a deep cut") + } +} + +func TestListOnThisDayTracks_WrapsYearBoundary(t *testing.T) { + // #1256: plain ABS on day-of-year made Dec 28 vs Jan 3 read as 359 + // days apart. The build date parameter anchors "today" so the wrap + // is testable at any run date. + pool := newPool(t) + q := dbq.New(pool) + u := seedUser(t, pool, "otd1") + + // Played Dec 28, 2025 — 6 circular days from the Jan 3 build date. + winter := seedTrack(t, pool, "otd1-winter", "otd1-artist") + seedPlayEvent(t, pool, u.ID, winter.ID, + time.Date(2025, 12, 28, 12, 0, 0, 0, time.UTC), false) + // Played mid-March — far outside the ±10-day window. + spring := seedTrack(t, pool, "otd1-spring", "otd1-artist") + seedPlayEvent(t, pool, u.ID, spring.ID, + time.Date(2025, 3, 15, 12, 0, 0, 0, time.UTC), false) + + rows, err := q.ListOnThisDayTracks(context.Background(), dbq.ListOnThisDayTracksParams{ + UserID: u.ID, Column2: "2026-01-03", + }) + if err != nil { + t.Fatalf("ListOnThisDayTracks: %v", err) + } + got := idSet(rows, func(r dbq.ListOnThisDayTracksRow) pgtype.UUID { return r.ID }) + if !got[winter.ID] { + t.Error("play 6 calendar days across the year boundary must qualify (circular DOY)") + } + if got[spring.ID] { + t.Error("play ~70 days away must not qualify") + } +} + +func TestListRediscoverTracks_SkippedResurfacingCoolsDown(t *testing.T) { + // #1258: a skip on a rediscover-sourced play is a declined + // invitation — the track sits out ~90 days instead of re-qualifying + // the next day. Skips from other surfaces don't cool down, and the + // cooldown expires. + pool := newPool(t) + q := dbq.New(pool) + u := seedUser(t, pool, "rd1") + now := time.Now().UTC() + + eligible := func(name string, playsAge time.Duration) dbq.Track { + tk := seedTrack(t, pool, name, name+"-artist") + for p := 0; p < 3; p++ { + seedPlayEvent(t, pool, u.ID, tk.ID, + now.Add(-playsAge-time.Duration(p)*time.Hour), false) + } + return tk + } + + // Declined 5 days ago from Rediscover itself → cooling down. + declined := eligible("rd1-declined", 60*24*time.Hour) + seedSourcedPlayEvent(t, pool, u.ID, declined.ID, now.Add(-5*24*time.Hour), true, "rediscover") + // Skipped 5 days ago, but from a different surface → still eligible. + otherSkip := eligible("rd1-otherskip", 60*24*time.Hour) + seedSourcedPlayEvent(t, pool, u.ID, otherSkip.ID, now.Add(-5*24*time.Hour), true, "radio") + // Declined 100 days ago → cooldown expired, eligible again. + lapsed := eligible("rd1-lapsed", 120*24*time.Hour) + seedSourcedPlayEvent(t, pool, u.ID, lapsed.ID, now.Add(-100*24*time.Hour), true, "rediscover") + + rows, err := q.ListRediscoverTracks(context.Background(), u.ID) + if err != nil { + t.Fatalf("ListRediscoverTracks: %v", err) + } + got := idSet(rows, func(r dbq.ListRediscoverTracksRow) pgtype.UUID { return r.ID }) + if got[declined.ID] { + t.Error("track declined from rediscover 5 days ago must be cooling down") + } + if !got[otherSkip.ID] { + t.Error("skip from a non-rediscover surface must not cool the track down") + } + if !got[lapsed.ID] { + t.Error("cooldown must expire after ~90 days") + } +}