From b2c6f6f0e92def333566db87dd1d02b1934310ea Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 10 Jun 2026 23:15:22 -0400 Subject: [PATCH] fix(server): apply skip rule on auto-close instead of force-hiding orphans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit autoClosePriorOpen hardcoded was_skipped=true for every orphaned play_event (a play_started whose play_ended never arrived, e.g. the client backgrounded mid-track). That hid fully-listened tracks from History — a play that sat open past its own length was capped to the track duration (ratio ~1) yet still flagged skipped. Observed live: History showed 3 plays for a day of listening because most rows were auto-closed orphans marked skipped. Now the auto-close applies the same skip rule as RecordPlayEnded to the duration-capped elapsed estimate: ratio >= threshold OR elapsed >= the duration floor -> a real play that lands in History; a genuine quick-abandon still classifies as a skip. Still writes no skip_events row, so the ambiguous auto-close never feeds the skip-ratio / recommendation signal. This is the server half. The client-side root cause (backgrounded track transitions never closed, orphaning the rows in the first place) is tracked separately. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/playevents/writer.go | 18 ++++++++++++------ internal/playevents/writer_test.go | 22 ++++++++++++++++++++-- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/internal/playevents/writer.go b/internal/playevents/writer.go index 9759d578..9d3a75f0 100644 --- a/internal/playevents/writer.go +++ b/internal/playevents/writer.go @@ -471,10 +471,15 @@ func (w *Writer) captureSessionVector( } // autoClosePriorOpen closes any open (ended_at IS NULL) play_event for the -// user. Sets ended_at = at, duration_played_ms = min(at - started_at, track -// duration), was_skipped = true. Skip rule is NOT applied — auto-closed -// rows are flagged skipped regardless because we don't know what the user -// actually heard. +// user. Sets ended_at = at and duration_played_ms = min(at - started_at, +// track duration) — our best estimate of how long the orphan played when +// the client never sent play_ended (e.g. backgrounded mid-track). The same +// skip rule as RecordPlayEnded is then applied to that estimate: an orphan +// that sat open past the track's length reads as ratio ~1 → a real play, so +// a fully-listened track that lost its close still lands in history instead +// of being force-hidden. Deliberately writes no skip_events row — an +// auto-close is an ambiguous signal, not a deliberate skip, and must not +// feed the skip-ratio / recommendation signal. func (w *Writer) autoClosePriorOpen( ctx context.Context, q *dbq.Queries, @@ -503,6 +508,7 @@ func (w *Writer) autoClosePriorOpen( if track.DurationMs > 0 { ratio = float64(elapsedMs) / float64(track.DurationMs) } + isSkip := ratio < w.skipMaxCompletionRatio && elapsedMs < w.skipMaxDurationPlayedMs ratioPtr := ratio durPtr := elapsedMs if _, err := q.UpdatePlayEventEnded(ctx, dbq.UpdatePlayEventEndedParams{ @@ -510,11 +516,11 @@ func (w *Writer) autoClosePriorOpen( EndedAt: pgtype.Timestamptz{Time: at, Valid: true}, DurationPlayedMs: &durPtr, CompletionRatio: &ratioPtr, - WasSkipped: true, + WasSkipped: isSkip, }); err != nil { return err } w.logger.Info("playevents: auto-closed prior open row", - "play_event_id", prior.ID, "elapsed_ms", elapsedMs) + "play_event_id", prior.ID, "elapsed_ms", elapsedMs, "was_skipped", isSkip) return nil } diff --git a/internal/playevents/writer_test.go b/internal/playevents/writer_test.go index bfb30dc7..e4133922 100644 --- a/internal/playevents/writer_test.go +++ b/internal/playevents/writer_test.go @@ -122,14 +122,32 @@ func TestRecordPlayStarted_AutoClosesPriorOpenRow(t *testing.T) { if !prior.EndedAt.Valid { t.Errorf("prior should be closed") } - if !prior.WasSkipped { - t.Errorf("prior should be marked was_skipped=true (auto-close convention)") + // 45s elapsed on a 200s track clears the 30s duration threshold, so the + // skip rule's AND fails -> NOT a skip. An orphan that played long enough + // to count must still land in history rather than being force-hidden. + if prior.WasSkipped { + t.Errorf("auto-close at 45s should apply the skip rule -> was_skipped=false") } if prior.DurationPlayedMs == nil || *prior.DurationPlayedMs != 45_000 { t.Errorf("duration_played_ms = %v, want 45000", prior.DurationPlayedMs) } } +func TestRecordPlayStarted_AutoCloseShortPlayIsSkip(t *testing.T) { + f := newFixture(t, 200_000) + t1 := time.Now().UTC() + first, _ := f.w.RecordPlayStarted(context.Background(), f.user, f.track, "c", t1) + // 5s elapsed: ratio 0.025 < 0.5 AND 5000 < 30000 -> both fail -> a skip. + t2 := t1.Add(5 * time.Second) + if _, err := f.w.RecordPlayStarted(context.Background(), f.user, f.track, "c", t2); err != nil { + t.Fatalf("second: %v", err) + } + prior, _ := f.q.GetPlayEventByID(context.Background(), first.PlayEventID) + if !prior.WasSkipped { + t.Errorf("auto-close at 5s should be was_skipped=true") + } +} + func TestRecordPlayStarted_AutoCloseCapsAtTrackDuration(t *testing.T) { f := newFixture(t, 60_000) // 60s track t1 := time.Now().UTC()