From 4fd394bead6f645a06992097a71b984333d799d1 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 8 Oct 2026 20:52:06 -0400 Subject: [PATCH] fix(playqueue): keep the current track when earlier tracks are missing RetrieveWithMediaFiles drops queue entries whose tracks are no longer in the library (purged, or in a library the user lost access to), but it kept the stored current index. That index then pointed one track further for each dropped track before it, so getPlayQueue, getPlayQueueByIndex and GET /api/queue reported the wrong song with the old position. The current index is now remapped to the same track in the filtered list. If the current track itself is gone, the queue moves to the next remaining track (or the last one) and restarts it. The cleaned queue is then saved back in the background, so partial PUT /api/queue updates use the same list the client sees. The save is a conditional UPDATE that only applies if the row is unchanged since it was read, so it never undoes a newer client save, and it is skipped if loading the tracks failed. Fixes #6297 --- persistence/playqueue_repository.go | 47 ++++++++++++-- persistence/playqueue_repository_test.go | 80 +++++++++++++++++------- 2 files changed, 99 insertions(+), 28 deletions(-) diff --git a/persistence/playqueue_repository.go b/persistence/playqueue_repository.go index 9dc520c44..4373bb987 100644 --- a/persistence/playqueue_repository.go +++ b/persistence/playqueue_repository.go @@ -80,10 +80,47 @@ func (r *playQueueRepository) RetrieveWithMediaFiles(ctx context.Context, userId var res playQueue err := r.queryOne(ctx, sel, &res) q := r.toModel(&res) - q.Items = r.loadTracks(ctx, q.Items) + stored := q.Items + var loadErr error + q.Items, loadErr = r.loadTracks(ctx, stored) + if loadErr == nil && len(q.Items) < len(stored) { + q.Current, q.Position = remapCurrent(stored, q.Items, q.Current, q.Position) + go r.saveCleanedQueue(context.WithoutCancel(ctx), res, r.fromModel(&q)) + } return &q, err } +// remapCurrent relies on loaded being stored in the same order, minus the missing tracks. If the current track +// is gone, it moves to the next remaining one (or the last) and restarts it. +func remapCurrent(stored, loaded model.MediaFiles, current int, position int64) (int, int64) { + kept := 0 + for i, t := range stored { + if kept >= len(loaded) || loaded[kept].ID != t.ID { + continue + } + if i >= current { + if i > current { + position = 0 + } + return kept, position + } + kept++ + } + return max(len(loaded)-1, 0), 0 +} + +// saveCleanedQueue only writes if the queue was not changed since it was read, so it never undoes a newer save. +func (r *playQueueRepository) saveCleanedQueue(ctx context.Context, read, cleaned playQueue) { + ctx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + upd := Update(r.tableName). + Set("items", cleaned.Items).Set("current", cleaned.Current).Set("position", cleaned.Position). + Where(Eq{"id": read.ID, "items": read.Items, "current": read.Current, "position": read.Position}) + if _, err := r.executeSQL(ctx, upd); err != nil { + log.Warn(ctx, "Could not remove missing tracks from playqueue", "user", read.UserID, err) + } +} + func (r *playQueueRepository) Retrieve(ctx context.Context, userId string) (*model.PlayQueue, error) { sel := r.newSelect(ctx).Columns("*").Where(Eq{"user_id": userId}) var res playQueue @@ -130,13 +167,14 @@ func (r *playQueueRepository) toModel(pq *playQueue) model.PlayQueue { // loadTracks loads the tracks from the database. It receives a list of track IDs and returns a list of MediaFiles // in the same order as the input list. -func (r *playQueueRepository) loadTracks(ctx context.Context, tracks model.MediaFiles) model.MediaFiles { +func (r *playQueueRepository) loadTracks(ctx context.Context, tracks model.MediaFiles) (model.MediaFiles, error) { if len(tracks) == 0 { - return nil + return nil, nil } mfRepo := NewMediaFileRepository(r.db) trackMap := map[string]model.MediaFile{} + var loadErr error // Create an iterator to collect all track IDs ids := slice.SeqFunc(tracks, func(t model.MediaFile) string { return t.ID }) @@ -148,6 +186,7 @@ func (r *playQueueRepository) loadTracks(ctx context.Context, tracks model.Media if err != nil { u := loggedUser(ctx) log.Error(ctx, "Could not load playqueue/bookmark's tracks", "user", u.UserName, err) + loadErr = err } for _, t := range tracks { trackMap[t.ID] = t @@ -162,7 +201,7 @@ func (r *playQueueRepository) loadTracks(ctx context.Context, tracks model.Media newTracks = append(newTracks, track) } } - return newTracks + return newTracks, loadErr } func (r *playQueueRepository) clearPlayQueue(ctx context.Context, userId string) error { diff --git a/persistence/playqueue_repository_test.go b/persistence/playqueue_repository_test.go index 877faddfc..5d18cdd86 100644 --- a/persistence/playqueue_repository_test.go +++ b/persistence/playqueue_repository_test.go @@ -20,7 +20,7 @@ var _ = Describe("PlayQueueRepository", func() { BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) - ctx = log.NewContext(context.TODO()) + ctx = log.NewContext(GinkgoT().Context()) ctx = request.WithUser(ctx, model.User{ID: "userid", UserName: "userid", IsAdmin: true}) repo = NewPlayQueueRepository(GetDBXBuilder()) }) @@ -311,36 +311,68 @@ var _ = Describe("PlayQueueRepository", func() { AssertPlayQueue(expected, actual) }) - It("does not return tracks if they don't exist in the DB", func() { - // Add a new song to the DB - newSong := songRadioactivity - newSong.ID = "temp-track" - newSong.Path = "/new-path" - mfRepo := NewMediaFileRepository(GetDBXBuilder()) + Context("when tracks are no longer in the library", func() { + var mfRepo model.MediaFileRepository + gone := songRadioactivity + gone.ID = "gone-track" + gone.Path = "/gone-track" - Expect(mfRepo.Put(ctx, &newSong)).To(Succeed()) + BeforeEach(func() { + mfRepo = NewMediaFileRepository(GetDBXBuilder()) + Expect(mfRepo.Put(ctx, &gone)).To(Succeed()) + DeferCleanup(func() { _ = mfRepo.Delete(ctx, gone.ID) }) + }) - // Create a playqueue with the new song - pq := aPlayQueue("userid", 0, 0, newSong, songAntenna) - Expect(repo.Store(ctx, pq)).To(Succeed()) + retrieveAfterRemoval := func(current int, position int64, items ...model.MediaFile) *model.PlayQueue { + Expect(repo.Store(ctx, aPlayQueue("userid", current, position, items...))).To(Succeed()) + Expect(mfRepo.Delete(ctx, gone.ID)).To(Succeed()) + actual, err := repo.RetrieveWithMediaFiles(ctx, "userid") + Expect(err).ToNot(HaveOccurred()) + return actual + } - // Retrieve the playqueue - actual, err := repo.RetrieveWithMediaFiles(ctx, "userid") - Expect(err).ToNot(HaveOccurred()) + DescribeTable("points to the right current track", + func(current int, items []model.MediaFile, expectedID string, expectedPosition int64) { + actual := retrieveAfterRemoval(current, 45, items...) - // The playqueue should contain both tracks - AssertPlayQueue(pq, actual) + Expect(actual.Items).To(HaveLen(len(items) - 1)) + Expect(actual.Items[actual.Current].ID).To(Equal(expectedID)) + Expect(actual.Position).To(Equal(expectedPosition)) + }, + Entry("when an earlier track is removed", 1, []model.MediaFile{gone, songComeTogether, songDayInALife}, songComeTogether.ID, int64(45)), + Entry("when a later track is removed", 0, []model.MediaFile{songComeTogether, gone}, songComeTogether.ID, int64(45)), + Entry("moving to the next track from the start when the current one is removed", 1, []model.MediaFile{songComeTogether, gone, songDayInALife}, songDayInALife.ID, int64(0)), + Entry("moving to the last track when the removed current one was the last", 1, []model.MediaFile{songComeTogether, gone}, songComeTogether.ID, int64(0)), + ) - // Delete the new song - Expect(mfRepo.Delete(ctx, "temp-track")).To(Succeed()) + It("saves the cleaned queue", func() { + retrieveAfterRemoval(1, 45, gone, songComeTogether, songDayInALife) - // Retrieve the playqueue - actual, err = repo.RetrieveWithMediaFiles(ctx, "userid") - Expect(err).ToNot(HaveOccurred()) + Eventually(func(g Gomega) { + stored, err := repo.Retrieve(ctx, "userid") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(stored.Items).To(HaveLen(2)) + g.Expect(stored.Items[0].ID).To(Equal(songComeTogether.ID)) + g.Expect(stored.Current).To(BeZero()) + g.Expect(stored.Position).To(Equal(int64(45))) + }).Should(Succeed()) + }) - // The playqueue should not contain the deleted track - Expect(actual.Items).To(HaveLen(1)) - Expect(actual.Items[0].ID).To(Equal(songAntenna.ID)) + It("does not overwrite a queue that changed after it was read", func() { + r := repo.(*playQueueRepository) + read := aPlayQueue("userid", 1, 45, gone, songComeTogether) + Expect(repo.Store(ctx, read)).To(Succeed()) + + By("Saving a new queue from a client before the cleanup runs") + Expect(repo.Store(ctx, aPlayQueue("userid", 0, 10, songAntenna))).To(Succeed()) + r.saveCleanedQueue(ctx, r.fromModel(read), playQueue{Items: songComeTogether.ID, Position: 45}) + + stored, err := repo.Retrieve(ctx, "userid") + Expect(err).ToNot(HaveOccurred()) + Expect(stored.Items).To(HaveLen(1)) + Expect(stored.Items[0].ID).To(Equal(songAntenna.ID)) + Expect(stored.Position).To(Equal(int64(10))) + }) }) })