fix(mixes): close three intent gaps found in the system-playlists audit

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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TsF3cNoKrqCYsU78cXC8U6
2026-07-03 08:47:01 -04:00
parent a670840114
commit 2be07ef271
3 changed files with 253 additions and 2 deletions
@@ -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
@@ -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
@@ -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")
+	}
+}