From b2c6f6f0e92def333566db87dd1d02b1934310ea Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 10 Jun 2026 23:15:22 -0400 Subject: [PATCH 1/2] 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() From 653ed95f14b4b6006747742cb8111f09cd223c6e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 11 Jun 2026 00:01:13 -0400 Subject: [PATCH 2/2] fix(android): always serialize event `type` so plays aren't dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The native client's /api/events requests omitted the `type` discriminator entirely. The app's Json is configured with encodeDefaults=false (AppModule), so a `type` left at its data-class default ("play_started" etc.) is never written to the wire. The server multiplexes on `type` and returns 400 "unknown event type" for an empty one, which PlayEventsReporter's catch swallows — and the play_started path has no offline fallback, so the play is lost with no trace. Net effect: EVERY native Android play event (started/ended/skipped/ offline) has 400'd since this code was written. Listening History only ever populated from the Flutter/web clients; as usage moved to the native app, History went sparse. Confirmed live in the server access log: POST /api/events -> 400 on every play, while reads 200. Force the discriminator onto the wire with @EncodeDefault(Mode.ALWAYS) on each request type's `type` field. Surgical (vs flipping encodeDefaults globally), and idiomatic for a constant-valued discriminator. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../minstrel/models/wire/EventsWire.kt | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/EventsWire.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/EventsWire.kt index f901b11b..74d34d96 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/EventsWire.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/EventsWire.kt @@ -1,5 +1,9 @@ +@file:OptIn(ExperimentalSerializationApi::class) + package com.fabledsword.minstrel.models.wire +import kotlinx.serialization.EncodeDefault +import kotlinx.serialization.ExperimentalSerializationApi import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable @@ -15,7 +19,11 @@ import kotlinx.serialization.Serializable @Serializable data class PlayStartedRequest( - val type: String = "play_started", + // Discriminator MUST stay on the wire. The app's Json has + // encodeDefaults=false, so without this a `type` left at its + // default is omitted entirely and the server 400s the event as + // "unknown event type" — silently dropping every native play. + @EncodeDefault(EncodeDefault.Mode.ALWAYS) val type: String = "play_started", @SerialName("track_id") val trackId: String, @SerialName("client_id") val clientId: String, val source: String? = null, @@ -28,21 +36,21 @@ data class PlayStartedResponse( @Serializable data class PlayEndedRequest( - val type: String = "play_ended", + @EncodeDefault(EncodeDefault.Mode.ALWAYS) val type: String = "play_ended", @SerialName("play_event_id") val playEventId: String, @SerialName("duration_played_ms") val durationPlayedMs: Long, ) @Serializable data class PlaySkippedRequest( - val type: String = "play_skipped", + @EncodeDefault(EncodeDefault.Mode.ALWAYS) val type: String = "play_skipped", @SerialName("play_event_id") val playEventId: String, @SerialName("position_ms") val positionMs: Long, ) @Serializable data class PlayOfflineRequest( - val type: String = "play_offline", + @EncodeDefault(EncodeDefault.Mode.ALWAYS) val type: String = "play_offline", @SerialName("track_id") val trackId: String, @SerialName("client_id") val clientId: String, val at: String,