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