diff --git a/internal/api/events.go b/internal/api/events.go index c7e49f16..150f534f 100644 --- a/internal/api/events.go +++ b/internal/api/events.go @@ -31,6 +31,26 @@ type playStartedResponse struct { SessionID string `json:"session_id"` } +// eventFutureSkew is the tolerated client-clock drift into the future +// before a timestamp is treated as bogus and replaced with now. +const eventFutureSkew = 5 * time.Minute + +// clampEventTime bounds a client-supplied event timestamp to sanity: +// no earlier than the account's creation (offline replays can +// legitimately be days old, but no play can predate the user) and no +// later than now + a small skew allowance. Unbounded client clocks +// previously let a skewed device write arbitrarily old plays, which +// poisoned Rediscover's "not played in 6 months" ordering (#1246). +func clampEventTime(at, userCreatedAt, now time.Time) time.Time { + if at.Before(userCreatedAt) { + return userCreatedAt + } + if at.After(now.Add(eventFutureSkew)) { + return now + } + return at +} + type okResponse struct { OK bool `json:"ok"` } @@ -52,7 +72,7 @@ func (h *handlers) handleEvents(w http.ResponseWriter, r *http.Request) { writeErr(w, apierror.BadRequest("bad_request", "invalid `at` timestamp")) return } - at = parsed + at = clampEventTime(parsed.UTC(), user.CreatedAt.Time, time.Now().UTC()) } clientID := "" if req.ClientID != nil { diff --git a/internal/db/dbq/system_mixes.sql.go b/internal/db/dbq/system_mixes.sql.go index 3ea0e5b9..ad304865 100644 --- a/internal/db/dbq/system_mixes.sql.go +++ b/internal/db/dbq/system_mixes.sql.go @@ -262,37 +262,19 @@ WITH stats AS ( FROM play_events pe WHERE pe.user_id = $1 AND pe.was_skipped = false GROUP BY pe.track_id -), -deep AS ( - SELECT t.id, t.album_id, t.artist_id, s.c, 0 AS tier - FROM tracks t - JOIN stats s ON s.track_id = t.id - WHERE s.c >= 5 - AND s.last_at <= now() - interval '6 months' - AND NOT EXISTS ( - SELECT 1 FROM lidarr_quarantine q - WHERE q.user_id = $1 AND q.track_id = t.id - ) -), -shallow AS ( - SELECT t.id, t.album_id, t.artist_id, s.c, 1 AS tier - FROM tracks t - JOIN stats s ON s.track_id = t.id - WHERE s.c >= 5 - AND s.last_at <= now() - interval '30 days' - AND NOT EXISTS ( - SELECT 1 FROM lidarr_quarantine q - WHERE q.user_id = $1 AND q.track_id = t.id - ) ) -SELECT id, album_id, artist_id - FROM ( - SELECT id, album_id, artist_id, c, tier FROM deep - UNION ALL - SELECT id, album_id, artist_id, c, tier FROM shallow - WHERE NOT EXISTS (SELECT 1 FROM deep) - ) u - ORDER BY tier, c DESC, id +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN stats s ON s.track_id = t.id + WHERE s.c >= 3 + AND s.last_at <= now() - interval '30 days' + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY (s.last_at <= now() - interval '6 months') DESC, + (s.c >= 5) DESC, + s.c DESC, t.id LIMIT 200 ` @@ -302,13 +284,17 @@ type ListRediscoverTracksRow struct { ArtistID pgtype.UUID } -// #420 Rediscover: tracks the user played a lot (>=5 non-skip) but -// has drifted away from. Tiered so a young library still gets a mix: -// -// tier 0 not played in the last 6 months (true rediscovery) -// tier 1 (only if tier 0 empty) not played in the last 30 days -// -// Ordered by historical affection. $1 user_id. +// #420 Rediscover: tracks the user played a lot but has drifted away +// from. One blended pool (issue #1246: the old two-tier UNION was +// all-or-nothing — a single ">=6 months cold" row suppressed the whole +// ">=30 days" tier, which is how a one-song playlist shipped; the 6mo +// tier was also a strict subset of the 30d tier). Eligibility is >=3 +// non-skip plays and >=30 days cold; the bar sits at 3 rather than 5 +// so a weeks-old library still fields a pool — on young histories the +// >=5-play tracks are precisely the ones still in rotation. Ordering +// prefers true rediscoveries (>=6 months cold), then strong affection +// (>=5 plays), then raw play count; the producer's minimum floor +// decides whether the pool is big enough to ship at all. $1 user_id. func (q *Queries) ListRediscoverTracks(ctx context.Context, userID pgtype.UUID) ([]ListRediscoverTracksRow, error) { rows, err := q.db.Query(ctx, listRediscoverTracks, userID) if err != nil { diff --git a/internal/db/queries/system_mixes.sql b/internal/db/queries/system_mixes.sql index b483fad0..13d3155b 100644 --- a/internal/db/queries/system_mixes.sql +++ b/internal/db/queries/system_mixes.sql @@ -37,47 +37,35 @@ SELECT t.id, t.album_id, t.artist_id LIMIT 200; -- name: ListRediscoverTracks :many --- #420 Rediscover: tracks the user played a lot (>=5 non-skip) but --- has drifted away from. Tiered so a young library still gets a mix: --- tier 0 not played in the last 6 months (true rediscovery) --- tier 1 (only if tier 0 empty) not played in the last 30 days --- Ordered by historical affection. $1 user_id. +-- #420 Rediscover: tracks the user played a lot but has drifted away +-- from. One blended pool (issue #1246: the old two-tier UNION was +-- all-or-nothing — a single ">=6 months cold" row suppressed the whole +-- ">=30 days" tier, which is how a one-song playlist shipped; the 6mo +-- tier was also a strict subset of the 30d tier). Eligibility is >=3 +-- non-skip plays and >=30 days cold; the bar sits at 3 rather than 5 +-- so a weeks-old library still fields a pool — on young histories the +-- >=5-play tracks are precisely the ones still in rotation. Ordering +-- prefers true rediscoveries (>=6 months cold), then strong affection +-- (>=5 plays), then raw play count; the producer's minimum floor +-- decides whether the pool is big enough to ship at all. $1 user_id. WITH stats AS ( SELECT pe.track_id, COUNT(*) AS c, MAX(pe.started_at) AS last_at FROM play_events pe WHERE pe.user_id = $1 AND pe.was_skipped = false GROUP BY pe.track_id -), -deep AS ( - SELECT t.id, t.album_id, t.artist_id, s.c, 0 AS tier - FROM tracks t - JOIN stats s ON s.track_id = t.id - WHERE s.c >= 5 - AND s.last_at <= now() - interval '6 months' - AND NOT EXISTS ( - SELECT 1 FROM lidarr_quarantine q - WHERE q.user_id = $1 AND q.track_id = t.id - ) -), -shallow AS ( - SELECT t.id, t.album_id, t.artist_id, s.c, 1 AS tier - FROM tracks t - JOIN stats s ON s.track_id = t.id - WHERE s.c >= 5 - AND s.last_at <= now() - interval '30 days' - AND NOT EXISTS ( - SELECT 1 FROM lidarr_quarantine q - WHERE q.user_id = $1 AND q.track_id = t.id - ) ) -SELECT id, album_id, artist_id - FROM ( - SELECT id, album_id, artist_id, c, tier FROM deep - UNION ALL - SELECT id, album_id, artist_id, c, tier FROM shallow - WHERE NOT EXISTS (SELECT 1 FROM deep) - ) u - ORDER BY tier, c DESC, id +SELECT t.id, t.album_id, t.artist_id + FROM tracks t + JOIN stats s ON s.track_id = t.id + WHERE s.c >= 3 + AND s.last_at <= now() - interval '30 days' + AND NOT EXISTS ( + SELECT 1 FROM lidarr_quarantine q + WHERE q.user_id = $1 AND q.track_id = t.id + ) + ORDER BY (s.last_at <= now() - interval '6 months') DESC, + (s.c >= 5) DESC, + s.c DESC, t.id LIMIT 200; -- name: ListNewForYouTracks :many diff --git a/internal/playlists/system_mixes.go b/internal/playlists/system_mixes.go index 4bd862e4..7a2c13e2 100644 --- a/internal/playlists/system_mixes.go +++ b/internal/playlists/system_mixes.go @@ -40,6 +40,17 @@ import ( // Discover so shuffle-on-play has a varied pool within a day. const discoveryMixLen = 100 +// Minimum viable mix sizes (issue #1246): below the floor the variant +// is withheld entirely so Home renders its "listen more to unlock" +// placeholder — a playlist with a couple of songs reads as a build +// bug, not a mix. Album-coherent mixes (NewForYou / FirstListens) get +// a lower floor because one legitimate new album (~5+ tracks) is a +// useful mix on its own; the scattered mixes need more to feel real. +const ( + discoveryMixMinLen = 15 + discoveryMixMinLenAlbum = 5 +) + // discoveryMixSpec describes one discovery mix. The unified producer // reads the spec and runs a single code path for all variants. type discoveryMixSpec struct { @@ -55,6 +66,11 @@ type discoveryMixSpec struct { // or when day-over-day stability is the intended UX (NewForYou). dailyRotate bool + // minLen is the minimum viable mix size: a finished pool below it + // is withheld (no playlist row) so the client renders the locked + // placeholder instead of a mix that looks built-wrong (#1246). + minLen int + // fetch returns the raw ranked rows. dateStr is supplied for // queries that accept it (passed as the second positional arg // historically); queries that don't accept it ignore the param. @@ -79,7 +95,13 @@ func produceDiscoveryMix(spec discoveryMixSpec) systemPlaylistProducer { if spec.dailyRotate { pool = rotateForDay(pool, userID, dateStr) } - return emit(spec.name, spec.variant, finishMix(pool, spec.diversify)), nil + tracks := finishMix(pool, spec.diversify) + if len(tracks) < spec.minLen { + logger.Info("system playlist: "+spec.variant+" below minimum viable size; withholding", + "user_id", uuidStringPL(userID), "pool", len(tracks), "min", spec.minLen) + return nil, nil + } + return emit(spec.name, spec.variant, tracks), nil } } @@ -109,6 +131,7 @@ var discoveryMixSpecs = []discoveryMixSpec{ { name: "Deep Cuts", variant: "deep_cuts", diversify: true, dailyRotate: false, // SQL day-keys via md5(id||$2) + minLen: discoveryMixMinLen, fetch: func(ctx context.Context, q *dbq.Queries, uid pgtype.UUID, ds string) ([]discoverTrack, error) { rows, err := q.ListDeepCutsTracks(ctx, dbq.ListDeepCutsTracksParams{ UserID: uid, Column2: ds, @@ -126,6 +149,7 @@ var discoveryMixSpecs = []discoveryMixSpec{ { name: "Rediscover", variant: "rediscover", diversify: true, dailyRotate: true, // SQL has no date arg + minLen: discoveryMixMinLen, fetch: func(ctx context.Context, q *dbq.Queries, uid pgtype.UUID, _ string) ([]discoverTrack, error) { rows, err := q.ListRediscoverTracks(ctx, uid) if err != nil { @@ -141,6 +165,7 @@ var discoveryMixSpecs = []discoveryMixSpec{ { name: "New for you", variant: "new_for_you", diversify: true, dailyRotate: true, // operator wants daily rotation on all deterministic mixes + minLen: discoveryMixMinLenAlbum, fetch: func(ctx context.Context, q *dbq.Queries, uid pgtype.UUID, _ string) ([]discoverTrack, error) { rows, err := q.ListNewForYouTracks(ctx, uid) if err != nil { @@ -156,6 +181,7 @@ var discoveryMixSpecs = []discoveryMixSpec{ { name: "On this day", variant: "on_this_day", diversify: true, dailyRotate: false, // SQL day-keys via md5(id||$2) + minLen: discoveryMixMinLen, fetch: func(ctx context.Context, q *dbq.Queries, uid pgtype.UUID, ds string) ([]discoverTrack, error) { rows, err := q.ListOnThisDayTracks(ctx, dbq.ListOnThisDayTracksParams{ UserID: uid, Column2: ds, @@ -173,6 +199,7 @@ var discoveryMixSpecs = []discoveryMixSpec{ { name: "First listens", variant: "first_listens", diversify: true, dailyRotate: true, // SQL has no date arg; daily rotate + diversity top-up + minLen: discoveryMixMinLenAlbum, fetch: func(ctx context.Context, q *dbq.Queries, uid pgtype.UUID, _ string) ([]discoverTrack, error) { rows, err := q.ListFirstListensTracks(ctx, uid) if err != nil {