From ca201d3f4e0926da396d4e1ea8ab9b8c52358bd6 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 11 Oct 2026 00:43:25 -0400 Subject: [PATCH] fix(playlists): keep smart playlist tracks when a refresh is interrupted A smart playlist refresh deleted its tracks and inserted the new ones as separate statements. A cancelled request (for example, a closed browser tab) could stop it in between, leaving the playlist with no tracks but its old song_count. The cover rebuild then never fired, because it treated song_count == 0 as "was empty". The delete, insert, counters and evaluated_at update now run in one transaction, and the delete's row count decides whether the playlist was empty, so playlists left empty by earlier interruptions also get their cover. Child playlists, the percentage-limit count and the query build run before the transaction, so it holds the write lock only for the playlist's own tracks. --- persistence/playlist_repository.go | 10 ++++ persistence/smart_playlist_repository.go | 49 ++++++++++++------- persistence/smart_playlist_repository_test.go | 28 +++++++++++ 3 files changed, 69 insertions(+), 18 deletions(-) diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 41b75266c..d490e6497 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -373,6 +373,16 @@ func (r *playlistRepository) enqueueCoverRebuild(ctx context.Context, id string) } } +func (r *playlistRepository) inTx(block func(tx *playlistRepository) error) error { + conn, ok := r.db.(*dbx.DB) + if !ok { + return block(r) // already inside a transaction + } + return conn.Transactional(func(tx *dbx.Tx) error { + return block(NewPlaylistRepository(tx).(*playlistRepository)) + }) +} + // tracksQuery is shared by loadTracks and GetCursor, so both hydrate rows identically. func (r *playlistRepository) tracksQuery(ctx context.Context, query SelectBuilder, id string) SelectBuilder { query = r.applyLibraryFilter(ctx, query, "f") diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 0ea33a318..2bd967f00 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -2,6 +2,7 @@ package persistence import ( "context" + "fmt" "slices" "time" @@ -39,14 +40,10 @@ func (r *playlistRepository) refreshSmartPlaylistTree(ctx context.Context, pls * log.Debug(ctx, "Refreshing smart playlist", "playlist", pls.Name, "id", pls.ID) start := time.Now() - del := Delete("playlist_tracks").Where(Eq{"playlist_id": pls.ID}) - if _, err := r.executeSQL(ctx, del); err != nil { - log.Error(ctx, "Error deleting old smart playlist tracks", "playlist", pls.Name, "id", pls.ID, err) - return false - } - rulesSQL := newSmartPlaylistCriteria(*pls.NormalizedRules(), withSmartPlaylistOwner(*usr)) + // Children refresh first, each in its own transaction, so the parent's holds the write lock only + // for its own tracks. if !r.refreshChildPlaylists(ctx, pls, rulesSQL, visited) { return false } @@ -62,35 +59,51 @@ func (r *playlistRepository) refreshSmartPlaylistTree(ctx context.Context, pls * return false } - insSql := Insert("playlist_tracks").Columns("id", "playlist_id", "media_file_id").Select(sq) - if _, err = r.executeSQL(ctx, insSql); err != nil { - log.Error(ctx, "Error refreshing smart playlist tracks", "playlist", pls.Name, "id", pls.ID, err) + // One transaction, so a refresh that fails or is cancelled keeps the previous tracks. Work on a + // copy, so a rolled-back refresh leaves pls as it was. + refreshed := *pls + err = r.inTx(func(tx *playlistRepository) error { return tx.replaceSmartPlaylistTracks(ctx, &refreshed, sq) }) + if err != nil { + log.Error(ctx, "Error refreshing smart playlist", "playlist", pls.Name, "id", pls.ID, err) return false } + *pls = refreshed + + log.Debug(ctx, "Refreshed playlist", "playlist", pls.Name, "id", pls.ID, "numTracks", pls.SongCount, "elapsed", time.Since(start)) + return true +} + +func (r *playlistRepository) replaceSmartPlaylistTracks(ctx context.Context, pls *model.Playlist, sq SelectBuilder) error { + // The delete goes first: a transaction that reads before its first write can fail on a busy DB + // without waiting. Its row count, not song_count, says whether the playlist had tracks. + prevTracks, err := r.executeSQL(ctx, Delete("playlist_tracks").Where(Eq{"playlist_id": pls.ID})) + if err != nil { + return fmt.Errorf("deleting old tracks: %w", err) + } + + insSql := Insert("playlist_tracks").Columns("id", "playlist_id", "media_file_id").Select(sq) + if _, err = r.executeSQL(ctx, insSql); err != nil { + return fmt.Errorf("inserting tracks: %w", err) + } - wasEmpty := pls.SongCount == 0 if err = r.refreshCounters(ctx, pls); err != nil { - log.Error(ctx, "Error updating smart playlist stats", "playlist", pls.Name, "id", pls.ID, err) - return false + return fmt.Errorf("updating stats: %w", err) } // Reuse the stamp refreshCounters just wrote, so evaluated_at and updated_at agree now := pls.UpdatedAt updSql := Update(r.tableName).Set("evaluated_at", now).Where(Eq{"id": pls.ID}) if _, err = r.executeSQL(ctx, updSql); err != nil { - log.Error(ctx, "Error updating smart playlist", "playlist", pls.Name, "id", pls.ID, err) - return false + return fmt.Errorf("updating evaluated_at: %w", err) } pls.EvaluatedAt = &now // A cover resolved while the playlist was empty has no grid. Rebuild only on that transition: // rebuilding on every refresh would re-sample the grid, and re-run a grid that has no art. - if wasEmpty && pls.SongCount > 0 { + if prevTracks == 0 && pls.SongCount > 0 { r.enqueueCoverRebuild(ctx, pls.ID) } - - log.Debug(ctx, "Refreshed playlist", "playlist", pls.Name, "id", pls.ID, "numTracks", pls.SongCount, "elapsed", time.Since(start)) - return true + return nil } // shouldRefreshSmartPlaylist determines if a smart playlist needs to be refreshed based on its type, last evaluated diff --git a/persistence/smart_playlist_repository_test.go b/persistence/smart_playlist_repository_test.go index 39e8ea050..65e294114 100644 --- a/persistence/smart_playlist_repository_test.go +++ b/persistence/smart_playlist_repository_test.go @@ -103,6 +103,34 @@ var _ = Describe("PlaylistRepository - Smart Playlists", func() { Expect(queuedIDs()).ToNot(ContainElement(pls.ID)) }) + + // Refreshes interrupted before the transaction existed left no tracks but the old song_count. + It("enqueues the cover when refilling a playlist whose previous refresh was interrupted", func() { + _, err := repo.GetWithTracks(ctx, pls.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + clearArtworkTables() + _, err = GetDBXBuilder().Delete("playlist_tracks", dbx.HashExp{"playlist_id": pls.ID}).Execute() + Expect(err).ToNot(HaveOccurred()) + + _, err = repo.GetWithTracks(ctx, pls.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + + Expect(queuedIDs()).To(ContainElement(pls.ID)) + }) + + It("keeps the previous tracks when a refresh fails after clearing them", func() { + before, err := repo.GetWithTracks(ctx, pls.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + _, err = GetDBXBuilder().NewQuery(`CREATE TRIGGER fail_refresh BEFORE INSERT ON playlist_tracks + WHEN NEW.playlist_id = '` + pls.ID + `' BEGIN SELECT RAISE(ABORT, 'refresh interrupted'); END`).Execute() + Expect(err).ToNot(HaveOccurred()) + DeferCleanup(func() { _, _ = GetDBXBuilder().NewQuery("DROP TRIGGER fail_refresh").Execute() }) + + after, err := repo.GetWithTracks(ctx, pls.ID, true, false) + Expect(err).ToNot(HaveOccurred()) + + Expect(after.Tracks).To(HaveLen(len(before.Tracks))) + }) }) Context("invalid rules", func() {