fix: harden offline/playback recording (contract-audit follow-ups)
test-go / test (push) Successful in 42s
android / Build + lint + test (push) Failing after 1m27s
test-go / integration (push) Successful in 4m42s

Audit of every Android↔server connection point (2026-06-11) cleared the
silent-contract class that caused the events `type` bug, but surfaced a
cluster of offline/playback-robustness defects. Fixes:

1. Playback double-count on background. PlayEventsReporter no longer
   enqueues a partial play_offline + leaves the live row open on every
   screen-lock. closeCurrent() now routes by whether the server has an
   open row: close-by-id (durable PLAY_ENDED on failure) when it does,
   offline only when no row exists, and a no-op while a play_started is
   in flight (the server auto-closes that orphan). onStop only durably
   closes a *paused* play — a still-playing one is left to the live path
   under the foreground service. Adds the PLAY_ENDED mutation kind.

2. Replayer poison rows. MutationReplayer now classifies each replay as
   SENT / DROP / RETRY: permanent 4xx (and corrupt payloads) are dropped
   instead of retried forever; 408/429/5xx/transport still retry.

3. Offline-play / close-by-id idempotency (server). RecordOfflinePlay
   dedups on (user, track, started_at); RecordPlayEnded skips a second
   skip_events insert when re-closing an already-ended row. Makes the
   at-least-once replay safe against lost-response duplicates.

4. Like-toggle collapse. Replayer drops like-toggles superseded by a
   later toggle for the same entity, so partial-failure + differential
   retry can't invert the final like state.

5. Connectivity-return trigger. MutationReplayer + SyncController now
   also drain/sync when NetworkStatusController recovers to Healthy, so
   an offline→online transition mid-session doesn't wait for a cold
   start. SyncController.syncSafe gains a single-in-flight mutex.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-06-11 12:05:35 -04:00
parent 653ed95f14
commit d2a22e49e3
6 changed files with 373 additions and 189 deletions
+25 -1
View File
@@ -180,6 +180,12 @@ func (w *Writer) RecordPlayEnded(
if err != nil {
return err
}
// Idempotency for the client's durable close-by-id replay: if the
// row is already closed, re-applying the close is harmless (the
// UPDATE is last-write-wins) but a second skip_events insert must
// NOT fire — a lost-response retry would otherwise double the
// skip signal. See the 2026-06-11 contract audit.
alreadyEnded := ev.EndedAt.Valid
ratio := 0.0
if track.DurationMs > 0 {
ratio = float64(durationPlayedMs) / float64(track.DurationMs)
@@ -196,7 +202,7 @@ func (w *Writer) RecordPlayEnded(
}); err != nil {
return err
}
if isSkip {
if isSkip && !alreadyEnded {
if _, err := q.InsertSkipEvent(ctx, dbq.InsertSkipEventParams{
UserID: ev.UserID,
TrackID: ev.TrackID,
@@ -356,6 +362,24 @@ func (w *Writer) RecordOfflinePlay(
if err != nil {
return err
}
// Idempotency: the offline queue is at-least-once, so a response lost
// after commit makes the client re-fire the same play. The replayed
// payload carries the original `at`, so an exact (user, track,
// started_at) match means we already recorded it — no-op instead of a
// duplicate history row. See the 2026-06-11 contract audit.
var dupID pgtype.UUID
err = tx.QueryRow(ctx,
`SELECT id FROM play_events
WHERE user_id = $1 AND track_id = $2 AND started_at = $3
LIMIT 1`,
userID, trackID, pgtype.Timestamptz{Time: at, Valid: true},
).Scan(&dupID)
if err == nil {
return nil
}
if !errors.Is(err, pgx.ErrNoRows) {
return err
}
if err := w.autoClosePriorOpen(ctx, q, userID, at); err != nil {
return err
}
+49
View File
@@ -219,6 +219,55 @@ func TestRecordPlayEnded_DurationOver50Percent_NotSkip(t *testing.T) {
}
}
// A lost-response retry of an offline play (same user/track/started_at)
// must not create a second history row. Guards the at-least-once replay
// path added in the 2026-06-11 audit.
func TestRecordOfflinePlay_DedupsByUserTrackStartedAt(t *testing.T) {
f := newFixture(t, 200_000)
ctx := context.Background()
at := time.Now().UTC()
if err := f.w.RecordOfflinePlay(ctx, f.user, f.track, "c", "", at, 120_000); err != nil {
t.Fatalf("first offline: %v", err)
}
if err := f.w.RecordOfflinePlay(ctx, f.user, f.track, "c", "", at, 120_000); err != nil {
t.Fatalf("replay offline: %v", err)
}
var count int
if err := f.pool.QueryRow(ctx,
"SELECT COUNT(*) FROM play_events WHERE user_id=$1 AND track_id=$2 AND started_at=$3",
f.user, f.track, pgtype.Timestamptz{Time: at, Valid: true}).Scan(&count); err != nil {
t.Fatalf("count: %v", err)
}
if count != 1 {
t.Errorf("play_events count = %d, want 1 (deduped)", count)
}
}
// Re-closing an already-ended row (the client's durable close-by-id replay)
// updates the row but must NOT insert a second skip_events row.
func TestRecordPlayEnded_IdempotentClose_NoDuplicateSkipEvent(t *testing.T) {
f := newFixture(t, 200_000)
ctx := context.Background()
t1 := time.Now().UTC()
res, _ := f.w.RecordPlayStarted(ctx, f.user, f.track, "c", t1)
t2 := t1.Add(10 * time.Second) // 5% / 10s -> skip
if err := f.w.RecordPlayEnded(ctx, res.PlayEventID, 10_000, t2); err != nil {
t.Fatalf("first end: %v", err)
}
if err := f.w.RecordPlayEnded(ctx, res.PlayEventID, 10_000, t2); err != nil {
t.Fatalf("replay end: %v", err)
}
var count int
if err := f.pool.QueryRow(ctx,
"SELECT COUNT(*) FROM skip_events WHERE user_id=$1 AND track_id=$2",
f.user, f.track).Scan(&count); err != nil {
t.Fatalf("count: %v", err)
}
if count != 1 {
t.Errorf("skip_events count = %d, want 1 (idempotent close)", count)
}
}
func TestRecordPlaySkipped_AlwaysSkipFlagAndSkipEventRow(t *testing.T) {
f := newFixture(t, 200_000)
t1 := time.Now().UTC()