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

@@ -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")

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

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() {