From 19365e5ad0a9887135045decc719d37ff436f07b Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 8 Nov 2025 15:10:00 -0500 Subject: [PATCH 1/5] feat: add album refresh functionality after deleting missing files Implemented RefreshAlbums method in AlbumRepository to recalculate album attributes (size, duration, song count) from their constituent media files. This method processes albums in batches to maintain efficiency with large datasets. Added integration in deleteMissingFiles to automatically refresh affected albums in the background after deleting missing media files, ensuring album statistics remain accurate. Includes comprehensive test coverage for various scenarios including single/multiple albums, empty batches, and large batch processing. Signed-off-by: Deluan --- model/album.go | 3 + persistence/album_repository.go | 88 ++++++++++++++++++++ persistence/album_repository_test.go | 117 +++++++++++++++++++++++++++ server/nativeapi/missing.go | 62 ++++++++++++++ 4 files changed, 270 insertions(+) diff --git a/model/album.go b/model/album.go index a8dcfe682..b391af591 100644 --- a/model/album.go +++ b/model/album.go @@ -139,6 +139,9 @@ type AlbumRepository interface { RefreshPlayCounts() (int64, error) CopyAttributes(fromID, toID string, columns ...string) error + // RefreshAlbums recalculates album attributes (size, duration, etc.) from media files + RefreshAlbums(albumIDs []string) error + AnnotatedRepository SearchableRepository[Albums] } diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 6f9bb3b48..113e3cd0e 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -337,6 +337,94 @@ on conflict (user_id, item_id, item_type) do update return r.executeSQL(query) } +// RefreshAlbums recalculates album attributes (size, duration, song count, etc.) from media files. +// It uses batch queries to minimize database round-trips for efficiency. +func (r *albumRepository) RefreshAlbums(albumIDs []string) error { + if len(albumIDs) == 0 { + return nil + } + + log.Debug(r.ctx, "Refreshing albums", "count", len(albumIDs)) + + // Process in chunks to avoid query size limits + const chunkSize = 100 + for i := 0; i < len(albumIDs); i += chunkSize { + end := i + chunkSize + if end > len(albumIDs) { + end = len(albumIDs) + } + chunk := albumIDs[i:end] + + if err := r.refreshAlbumChunk(chunk); err != nil { + return fmt.Errorf("refreshing album chunk: %w", err) + } + } + + log.Debug(r.ctx, "Successfully refreshed albums", "count", len(albumIDs)) + return nil +} + +// refreshAlbumChunk processes a single chunk of album IDs +func (r *albumRepository) refreshAlbumChunk(albumIDs []string) error { + // Batch load existing albums + albums, err := r.GetAll(model.QueryOptions{Filters: Eq{"album.id": albumIDs}}) + if err != nil { + return fmt.Errorf("loading albums: %w", err) + } + + // Create a map for quick lookup + albumMap := make(map[string]*model.Album, len(albums)) + for i := range albums { + albumMap[albums[i].ID] = &albums[i] + } + + // Batch load all media files for these albums using MediaFile repository + mfRepo := NewMediaFileRepository(r.ctx, r.db) + mediaFiles, err := mfRepo.GetAll(model.QueryOptions{ + Filters: Eq{"album_id": albumIDs}, + Sort: "album_id, path", + }) + if err != nil { + return fmt.Errorf("loading media files: %w", err) + } + + // Group media files by album ID + filesByAlbum := make(map[string]model.MediaFiles) + for i := range mediaFiles { + albumID := mediaFiles[i].AlbumID + filesByAlbum[albumID] = append(filesByAlbum[albumID], mediaFiles[i]) + } + + // Recalculate each album from its media files + for albumID, oldAlbum := range albumMap { + mfs, hasTracks := filesByAlbum[albumID] + if !hasTracks { + // Album has no tracks anymore, skip (will be cleaned up by GC) + log.Debug(r.ctx, "Skipping album with no tracks", "albumID", albumID) + continue + } + + // Recalculate album from media files + newAlbum := mfs.ToAlbum() + + // Only update if something changed (avoid unnecessary writes) + if !oldAlbum.Equals(newAlbum) { + // Preserve original timestamps + newAlbum.UpdatedAt = time.Now() + newAlbum.CreatedAt = oldAlbum.CreatedAt + + if err := r.Put(&newAlbum); err != nil { + log.Error(r.ctx, "Error updating album during refresh", "albumID", albumID, err) + // Continue with other albums instead of failing entirely + continue + } + log.Trace(r.ctx, "Refreshed album", "albumID", albumID, "name", newAlbum.Name) + } + } + + return nil +} + func (r *albumRepository) purgeEmpty() error { del := Delete(r.tableName).Where("id not in (select distinct(album_id) from media_file)") c, err := r.executeSQL(del) diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index a062b4398..11d9a8d47 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -513,6 +513,123 @@ var _ = Describe("AlbumRepository", func() { _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID})) }) }) + + Describe("RefreshAlbums", func() { + var mfRepo *mediaFileRepository + + BeforeEach(func() { + ctx := request.WithUser(GinkgoT().Context(), adminUser) + albumRepo = NewAlbumRepository(ctx, GetDBXBuilder()).(*albumRepository) + mfRepo = NewMediaFileRepository(ctx, GetDBXBuilder()).(*mediaFileRepository) + }) + + It("recalculates size and duration after files are modified", func() { + // Get the initial album + album, err := albumRepo.Get("103") // Radioactivity album + Expect(err).ToNot(HaveOccurred()) + initialSize := album.Size + initialDuration := album.Duration + + // Modify the size and duration of one of the media files + mf, err := mfRepo.Get("1003") // Radioactivity song + Expect(err).ToNot(HaveOccurred()) + mf.Size = 5000000 // 5MB + mf.Duration = 300.5 // 5 minutes + Expect(mfRepo.Put(mf)).To(Succeed()) + + // Refresh the album + err = albumRepo.RefreshAlbums([]string{"103"}) + Expect(err).ToNot(HaveOccurred()) + + // Verify the album was refreshed with new values + refreshedAlbum, err := albumRepo.Get("103") + Expect(err).ToNot(HaveOccurred()) + Expect(refreshedAlbum.Size).ToNot(Equal(initialSize)) + Expect(refreshedAlbum.Duration).ToNot(Equal(initialDuration)) + }) + + It("handles multiple albums in a single call", func() { + // Modify files in two different albums + mf1, err := mfRepo.Get("1001") // Sgt Peppers song + Expect(err).ToNot(HaveOccurred()) + mf1.Size = 3000000 + Expect(mfRepo.Put(mf1)).To(Succeed()) + + mf2, err := mfRepo.Get("1002") // Abbey Road song + Expect(err).ToNot(HaveOccurred()) + mf2.Size = 4000000 + Expect(mfRepo.Put(mf2)).To(Succeed()) + + // Refresh both albums in one call + err = albumRepo.RefreshAlbums([]string{"101", "102"}) + Expect(err).ToNot(HaveOccurred()) + + // Verify both were refreshed + album1, err := albumRepo.Get("101") + Expect(err).ToNot(HaveOccurred()) + Expect(album1.Size).To(Equal(int64(3000000))) + + album2, err := albumRepo.Get("102") + Expect(err).ToNot(HaveOccurred()) + Expect(album2.Size).To(Equal(int64(4000000))) + }) + + It("handles empty album ID list gracefully", func() { + err := albumRepo.RefreshAlbums([]string{}) + Expect(err).ToNot(HaveOccurred()) + }) + + It("handles non-existent album IDs gracefully", func() { + err := albumRepo.RefreshAlbums([]string{"non-existent-id"}) + Expect(err).ToNot(HaveOccurred()) + }) + + It("recalculates song count correctly", func() { + album, err := albumRepo.Get("103") // Radioactivity album + Expect(err).ToNot(HaveOccurred()) + initialSongCount := album.SongCount + + // Add a new media file to this album + newSong := mf(model.MediaFile{ + ID: "1099", + Title: "New Song", + ArtistID: "2", + Artist: "Kraftwerk", + AlbumID: "103", + Album: "Radioactivity", + Path: p("/kraft/radio/new-song.mp3"), + Size: 1000000, + Duration: 180, + }) + Expect(mfRepo.Put(&newSong)).To(Succeed()) + + // Refresh the album + err = albumRepo.RefreshAlbums([]string{"103"}) + Expect(err).ToNot(HaveOccurred()) + + // Verify song count increased + refreshedAlbum, err := albumRepo.Get("103") + Expect(err).ToNot(HaveOccurred()) + Expect(refreshedAlbum.SongCount).To(Equal(initialSongCount + 1)) + + // Clean up + _, _ = mfRepo.executeSQL(squirrel.Delete("media_file").Where(squirrel.Eq{"id": "1099"})) + }) + + It("processes large batches efficiently", func() { + // Test with all existing albums + allAlbums := []string{"101", "102", "103", "104"} + err := albumRepo.RefreshAlbums(allAlbums) + Expect(err).ToNot(HaveOccurred()) + + // Verify all albums still exist and have correct data + for _, albumID := range allAlbums { + album, err := albumRepo.Get(albumID) + Expect(err).ToNot(HaveOccurred()) + Expect(album.ID).To(Equal(albumID)) + } + }) + }) }) func _p(id, name string, sortName ...string) model.Participant { diff --git a/server/nativeapi/missing.go b/server/nativeapi/missing.go index 0d311f492..0c7b2688f 100644 --- a/server/nativeapi/missing.go +++ b/server/nativeapi/missing.go @@ -67,6 +67,22 @@ func deleteMissingFiles(ds model.DataStore, w http.ResponseWriter, r *http.Reque ctx := r.Context() p := req.Params(r) ids, _ := p.Strings("id") + + // Track affected album IDs before deletion for refresh + var affectedAlbumIDs []string + var trackErr error + if len(ids) == 0 { + // Get all album IDs from missing files + affectedAlbumIDs, trackErr = getAlbumIDsFromMissing(ctx, ds, nil) + } else { + // Get album IDs from specific missing file IDs + affectedAlbumIDs, trackErr = getAlbumIDsFromMissing(ctx, ds, ids) + } + if trackErr != nil { + log.Warn(ctx, "Error tracking affected albums for refresh", trackErr) + // Don't fail the operation, just log the warning + } + err := ds.WithTx(func(tx model.DataStore) error { if len(ids) == 0 { _, err := tx.MediaFile(ctx).DeleteAllMissing() @@ -101,7 +117,53 @@ func deleteMissingFiles(ds model.DataStore, w http.ResponseWriter, r *http.Reque } }() + // Refresh album stats in background after deleting missing files + if len(affectedAlbumIDs) > 0 { + go func() { + bgCtx := request.AddValues(context.Background(), r.Context()) + if err := ds.Album(bgCtx).RefreshAlbums(affectedAlbumIDs); err != nil { + log.Error(bgCtx, "Error refreshing album stats after deleting missing files", err) + } else { + log.Debug(bgCtx, "Successfully refreshed album stats after deleting missing files", "count", len(affectedAlbumIDs)) + } + }() + } + writeDeleteManyResponse(w, r, ids) } +// getAlbumIDsFromMissing returns distinct album IDs from missing media files +// Uses batch query for efficiency +func getAlbumIDsFromMissing(ctx context.Context, ds model.DataStore, ids []string) ([]string, error) { + var filters squirrel.Sqlizer = squirrel.Eq{"missing": true} + if len(ids) > 0 { + filters = squirrel.And{ + squirrel.Eq{"missing": true}, + squirrel.Eq{"id": ids}, + } + } + + mfs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: filters, + }) + if err != nil { + return nil, err + } + + // Extract unique album IDs + albumIDMap := make(map[string]struct{}, len(mfs)) + for _, mf := range mfs { + if mf.AlbumID != "" { + albumIDMap[mf.AlbumID] = struct{}{} + } + } + + albumIDs := make([]string, 0, len(albumIDMap)) + for id := range albumIDMap { + albumIDs = append(albumIDs, id) + } + + return albumIDs, nil +} + var _ model.ResourceRepository = &missingRepository{} From 593b5db8e9b24256c7d34a6ca9878a0db34318ae Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 8 Nov 2025 15:50:43 -0500 Subject: [PATCH 2/5] refactor: extract missing files deletion into reusable service layer Extracted inline deletion logic from server/nativeapi/missing.go into a new core.MissingFiles service interface and implementation. This provides better separation of concerns and testability. The MissingFiles service handles: - Deletion of specific or all missing files via transaction - Garbage collection after deletion - Extraction of affected album IDs from missing files - Background refresh of artist and album statistics The deleteMissingFiles HTTP handler now simply delegates to the service, removing 70+ lines of inline logic. All deletion, transaction, and stat refresh logic is now centralized in core/missing_files.go. Updated dependency injection to provide MissingFiles service to the native API router. Renamed receiver variable from 'n' to 'api' throughout native_api.go for consistency. --- cmd/wire_gen.go | 3 +- core/missing_files.go | 124 +++++++++++ core/missing_files_test.go | 262 +++++++++++++++++++++++ core/wire_providers.go | 1 + server/nativeapi/config_test.go | 2 +- server/nativeapi/library.go | 6 +- server/nativeapi/library_test.go | 2 +- server/nativeapi/missing.go | 109 ++-------- server/nativeapi/native_api.go | 127 ++++++----- server/nativeapi/native_api_song_test.go | 2 +- 10 files changed, 475 insertions(+), 163 deletions(-) create mode 100644 core/missing_files.go create mode 100644 core/missing_files_test.go diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 187ab488d..31388780e 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -72,7 +72,8 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { scannerScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, scannerScanner) library := core.NewLibrary(dataStore, scannerScanner, watcher, broker) - router := nativeapi.New(dataStore, share, playlists, insights, library) + missingFiles := core.NewMissingFiles(dataStore) + router := nativeapi.New(dataStore, share, playlists, insights, library, missingFiles) return router } diff --git a/core/missing_files.go b/core/missing_files.go new file mode 100644 index 000000000..19b440f4f --- /dev/null +++ b/core/missing_files.go @@ -0,0 +1,124 @@ +package core + +import ( + "context" + + "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" +) + +type MissingFiles interface { + // DeleteMissingFiles deletes specific missing files by their IDs + DeleteMissingFiles(ctx context.Context, ids []string) error + // DeleteAllMissingFiles deletes all files marked as missing + DeleteAllMissingFiles(ctx context.Context) error +} + +type missingFilesService struct { + ds model.DataStore +} + +func NewMissingFiles(ds model.DataStore) MissingFiles { + return &missingFilesService{ + ds: ds, + } +} + +func (s *missingFilesService) DeleteMissingFiles(ctx context.Context, ids []string) error { + return s.deleteMissing(ctx, ids) +} + +func (s *missingFilesService) DeleteAllMissingFiles(ctx context.Context) error { + return s.deleteMissing(ctx, nil) +} + +// deleteMissing handles the deletion of missing files and triggers necessary cleanup operations +func (s *missingFilesService) deleteMissing(ctx context.Context, ids []string) error { + // Track affected album IDs before deletion for refresh + affectedAlbumIDs, err := s.getAffectedAlbumIDs(ctx, ids) + if err != nil { + log.Warn(ctx, "Error tracking affected albums for refresh", err) + // Don't fail the operation, just log the warning + } + + // Delete missing files within a transaction + err = s.ds.WithTx(func(tx model.DataStore) error { + if len(ids) == 0 { + _, err := tx.MediaFile(ctx).DeleteAllMissing() + return err + } + return tx.MediaFile(ctx).DeleteMissing(ids) + }) + if err != nil { + log.Error(ctx, "Error deleting missing tracks from DB", "ids", ids, err) + return err + } + + // Run garbage collection to clean up orphaned records + if err := s.ds.GC(ctx); err != nil { + log.Error(ctx, "Error running GC after deleting missing tracks", err) + return err + } + + // Refresh statistics in background + s.refreshStatsAsync(ctx, affectedAlbumIDs) + + return nil +} + +// getAffectedAlbumIDs returns distinct album IDs from missing media files +func (s *missingFilesService) getAffectedAlbumIDs(ctx context.Context, ids []string) ([]string, error) { + var filters squirrel.Sqlizer = squirrel.Eq{"missing": true} + if len(ids) > 0 { + filters = squirrel.And{ + squirrel.Eq{"missing": true}, + squirrel.Eq{"id": ids}, + } + } + + mfs, err := s.ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: filters, + }) + if err != nil { + return nil, err + } + + // Extract unique album IDs + albumIDMap := make(map[string]struct{}, len(mfs)) + for _, mf := range mfs { + if mf.AlbumID != "" { + albumIDMap[mf.AlbumID] = struct{}{} + } + } + + albumIDs := make([]string, 0, len(albumIDMap)) + for id := range albumIDMap { + albumIDs = append(albumIDs, id) + } + + return albumIDs, nil +} + +// refreshStatsAsync refreshes artist and album statistics in background goroutines +func (s *missingFilesService) refreshStatsAsync(ctx context.Context, affectedAlbumIDs []string) { + // Refresh artist stats in background + go func() { + bgCtx := request.AddValues(context.Background(), ctx) + if _, err := s.ds.Artist(bgCtx).RefreshStats(true); err != nil { + log.Error(bgCtx, "Error refreshing artist stats after deleting missing files", err) + } else { + log.Debug(bgCtx, "Successfully refreshed artist stats after deleting missing files") + } + + // Refresh album stats in background if we have affected albums + if len(affectedAlbumIDs) > 0 { + if err := s.ds.Album(bgCtx).RefreshAlbums(affectedAlbumIDs); err != nil { + log.Error(bgCtx, "Error refreshing album stats after deleting missing files", err) + } else { + log.Debug(bgCtx, "Successfully refreshed album stats after deleting missing files", "count", len(affectedAlbumIDs)) + } + } + }() +} diff --git a/core/missing_files_test.go b/core/missing_files_test.go new file mode 100644 index 000000000..3ea6316f3 --- /dev/null +++ b/core/missing_files_test.go @@ -0,0 +1,262 @@ +package core + +import ( + "context" + "errors" + + "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("MissingFiles", func() { + var ds *testDataStore + var service MissingFiles + var ctx context.Context + + BeforeEach(func() { + ctx = context.Background() + ctx = request.WithUser(ctx, model.User{ID: "user1", IsAdmin: true}) + + ds = &testDataStore{ + mfRepo: &testMediaFileRepo{}, + albumRepo: &testAlbumRepo{}, + artistRepo: &testArtistRepo{}, + } + + service = NewMissingFiles(ds) + }) + + Describe("DeleteMissingFiles", func() { + Context("with specific IDs", func() { + It("deletes specific missing files", func() { + // Setup: mock missing files with album IDs + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album2", Missing: true}, + } + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) + + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.deleteMissingCalled).To(BeTrue()) + Expect(ds.mfRepo.deletedIDs).To(Equal([]string{"mf1", "mf2"})) + Expect(ds.gcCalled).To(BeTrue()) + }) + + It("returns error if deletion fails", func() { + ds.mfRepo.deleteMissingError = errors.New("delete failed") + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("delete failed")) + }) + + It("continues even if album tracking fails", func() { + ds.mfRepo.getAllError = errors.New("tracking failed") + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + // Should not fail, just log warning + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.deleteMissingCalled).To(BeTrue()) + }) + + It("returns error if GC fails", func() { + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + } + ds.gcError = errors.New("gc failed") + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("gc failed")) + }) + }) + + Context("album ID extraction", func() { + It("extracts unique album IDs from missing files", func() { + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album1", Missing: true}, + {ID: "mf3", AlbumID: "album2", Missing: true}, + } + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2", "mf3"}) + + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.getAllCalled).To(BeTrue()) + }) + + It("skips files without album IDs", func() { + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "", Missing: true}, + {ID: "mf2", AlbumID: "album1", Missing: true}, + } + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) + + Expect(err).ToNot(HaveOccurred()) + }) + }) + }) + + Describe("DeleteAllMissingFiles", func() { + It("deletes all missing files", func() { + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album2", Missing: true}, + {ID: "mf3", AlbumID: "album3", Missing: true}, + } + + err := service.DeleteAllMissingFiles(ctx) + + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.deleteAllMissingCalled).To(BeTrue()) + Expect(ds.gcCalled).To(BeTrue()) + }) + + It("returns error if deletion fails", func() { + ds.mfRepo.deleteAllMissingError = errors.New("delete all failed") + + err := service.DeleteAllMissingFiles(ctx) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("delete all failed")) + }) + + It("handles empty result gracefully", func() { + ds.mfRepo.files = model.MediaFiles{} + + err := service.DeleteAllMissingFiles(ctx) + + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.deleteAllMissingCalled).To(BeTrue()) + }) + }) +}) + +// Test implementations +type testDataStore struct { + tests.MockDataStore + mfRepo *testMediaFileRepo + albumRepo *testAlbumRepo + artistRepo *testArtistRepo + gcCalled bool + gcError error +} + +func (ds *testDataStore) MediaFile(ctx context.Context) model.MediaFileRepository { + return ds.mfRepo +} + +func (ds *testDataStore) Album(ctx context.Context) model.AlbumRepository { + return ds.albumRepo +} + +func (ds *testDataStore) Artist(ctx context.Context) model.ArtistRepository { + return ds.artistRepo +} + +func (ds *testDataStore) WithTx(block func(tx model.DataStore) error, label ...string) error { + return block(ds) +} + +func (ds *testDataStore) GC(ctx context.Context) error { + ds.gcCalled = true + return ds.gcError +} + +type testMediaFileRepo struct { + tests.MockMediaFileRepo + files model.MediaFiles + getAllCalled bool + getAllError error + deleteMissingCalled bool + deletedIDs []string + deleteMissingError error + deleteAllMissingCalled bool + deleteAllMissingError error +} + +func (m *testMediaFileRepo) GetAll(options ...model.QueryOptions) (model.MediaFiles, error) { + m.getAllCalled = true + if m.getAllError != nil { + return nil, m.getAllError + } + + if len(options) == 0 { + return m.files, nil + } + + // Filter based on the query options + opt := options[0] + if filters, ok := opt.Filters.(squirrel.And); ok { + // Check for ID filter + for _, filter := range filters { + if eq, ok := filter.(squirrel.Eq); ok { + if ids, exists := eq["id"]; exists { + // Filter files by IDs + idList := ids.([]string) + var filtered model.MediaFiles + for _, f := range m.files { + for _, id := range idList { + if f.ID == id { + filtered = append(filtered, f) + break + } + } + } + return filtered, nil + } + } + } + } + return m.files, nil +} + +func (m *testMediaFileRepo) DeleteMissing(ids []string) error { + m.deleteMissingCalled = true + m.deletedIDs = ids + return m.deleteMissingError +} + +func (m *testMediaFileRepo) DeleteAllMissing() (int64, error) { + m.deleteAllMissingCalled = true + if m.deleteAllMissingError != nil { + return 0, m.deleteAllMissingError + } + return int64(len(m.files)), nil +} + +type testAlbumRepo struct { + tests.MockAlbumRepo + refreshAlbumsCalled bool + refreshAlbumsIDs []string + refreshAlbumsError error +} + +func (m *testAlbumRepo) RefreshAlbums(albumIDs []string) error { + m.refreshAlbumsCalled = true + m.refreshAlbumsIDs = albumIDs + return m.refreshAlbumsError +} + +type testArtistRepo struct { + tests.MockArtistRepo + refreshStatsCalled bool + refreshStatsError error +} + +func (m *testArtistRepo) RefreshStats(allArtists bool) (int64, error) { + m.refreshStatsCalled = true + if m.refreshStatsError != nil { + return 0, m.refreshStatsError + } + return 1, nil +} diff --git a/core/wire_providers.go b/core/wire_providers.go index ae365156a..fb0b11d8b 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -18,6 +18,7 @@ var Set = wire.NewSet( NewShare, NewPlaylists, NewLibrary, + NewMissingFiles, agents.GetAgents, external.NewProvider, wire.Bind(new(external.Agents), new(*agents.Agents)), diff --git a/server/nativeapi/config_test.go b/server/nativeapi/config_test.go index 60f7c3394..d9c722955 100644 --- a/server/nativeapi/config_test.go +++ b/server/nativeapi/config_test.go @@ -29,7 +29,7 @@ var _ = Describe("Config API", func() { conf.Server.DevUIShowConfig = true // Enable config endpoint for tests ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService()) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/library.go b/server/nativeapi/library.go index f081eca78..1636e1dbb 100644 --- a/server/nativeapi/library.go +++ b/server/nativeapi/library.go @@ -13,11 +13,11 @@ import ( ) // User-library association endpoints (admin only) -func (n *Router) addUserLibraryRoute(r chi.Router) { +func (api *Router) addUserLibraryRoute(r chi.Router) { r.Route("/user/{id}/library", func(r chi.Router) { r.Use(parseUserIDMiddleware) - r.Get("/", getUserLibraries(n.libs)) - r.Put("/", setUserLibraries(n.libs)) + r.Get("/", getUserLibraries(api.libs)) + r.Put("/", setUserLibraries(api.libs)) }) } diff --git a/server/nativeapi/library_test.go b/server/nativeapi/library_test.go index 4e6d34582..950338492 100644 --- a/server/nativeapi/library_test.go +++ b/server/nativeapi/library_test.go @@ -30,7 +30,7 @@ var _ = Describe("Library API", func() { DeferCleanup(configtest.SetupConfig()) ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService()) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/missing.go b/server/nativeapi/missing.go index 0c7b2688f..d9109bb0d 100644 --- a/server/nativeapi/missing.go +++ b/server/nativeapi/missing.go @@ -8,9 +8,9 @@ import ( "github.com/Masterminds/squirrel" "github.com/deluan/rest" + "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/utils/req" ) @@ -63,107 +63,32 @@ func (r *missingRepository) EntityName() string { return "missing_files" } -func deleteMissingFiles(ds model.DataStore, w http.ResponseWriter, r *http.Request) { - ctx := r.Context() - p := req.Params(r) - ids, _ := p.Strings("id") +func deleteMissingFiles(missingFiles core.MissingFiles) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() - // Track affected album IDs before deletion for refresh - var affectedAlbumIDs []string - var trackErr error - if len(ids) == 0 { - // Get all album IDs from missing files - affectedAlbumIDs, trackErr = getAlbumIDsFromMissing(ctx, ds, nil) - } else { - // Get album IDs from specific missing file IDs - affectedAlbumIDs, trackErr = getAlbumIDsFromMissing(ctx, ds, ids) - } - if trackErr != nil { - log.Warn(ctx, "Error tracking affected albums for refresh", trackErr) - // Don't fail the operation, just log the warning - } + p := req.Params(r) + ids, _ := p.Strings("id") - err := ds.WithTx(func(tx model.DataStore) error { + var err error if len(ids) == 0 { - _, err := tx.MediaFile(ctx).DeleteAllMissing() - return err - } - return tx.MediaFile(ctx).DeleteMissing(ids) - }) - if len(ids) == 1 && errors.Is(err, model.ErrNotFound) { - log.Warn(ctx, "Missing file not found", "id", ids[0]) - http.Error(w, "not found", http.StatusNotFound) - return - } - if err != nil { - log.Error(ctx, "Error deleting missing tracks from DB", "ids", ids, err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - err = ds.GC(ctx) - if err != nil { - log.Error(ctx, "Error running GC after deleting missing tracks", err) - http.Error(w, err.Error(), http.StatusInternalServerError) - return - } - - // Refresh artist stats in background after deleting missing files - go func() { - bgCtx := request.AddValues(context.Background(), r.Context()) - if _, err := ds.Artist(bgCtx).RefreshStats(true); err != nil { - log.Error(bgCtx, "Error refreshing artist stats after deleting missing files", err) + err = missingFiles.DeleteAllMissingFiles(ctx) } else { - log.Debug(bgCtx, "Successfully refreshed artist stats after deleting missing files") + err = missingFiles.DeleteMissingFiles(ctx, ids) } - }() - // Refresh album stats in background after deleting missing files - if len(affectedAlbumIDs) > 0 { - go func() { - bgCtx := request.AddValues(context.Background(), r.Context()) - if err := ds.Album(bgCtx).RefreshAlbums(affectedAlbumIDs); err != nil { - log.Error(bgCtx, "Error refreshing album stats after deleting missing files", err) - } else { - log.Debug(bgCtx, "Successfully refreshed album stats after deleting missing files", "count", len(affectedAlbumIDs)) - } - }() - } - - writeDeleteManyResponse(w, r, ids) -} - -// getAlbumIDsFromMissing returns distinct album IDs from missing media files -// Uses batch query for efficiency -func getAlbumIDsFromMissing(ctx context.Context, ds model.DataStore, ids []string) ([]string, error) { - var filters squirrel.Sqlizer = squirrel.Eq{"missing": true} - if len(ids) > 0 { - filters = squirrel.And{ - squirrel.Eq{"missing": true}, - squirrel.Eq{"id": ids}, + if len(ids) == 1 && errors.Is(err, model.ErrNotFound) { + log.Warn(ctx, "Missing file not found", "id", ids[0]) + http.Error(w, "not found", http.StatusNotFound) + return } - } - - mfs, err := ds.MediaFile(ctx).GetAll(model.QueryOptions{ - Filters: filters, - }) - if err != nil { - return nil, err - } - - // Extract unique album IDs - albumIDMap := make(map[string]struct{}, len(mfs)) - for _, mf := range mfs { - if mf.AlbumID != "" { - albumIDMap[mf.AlbumID] = struct{}{} + if err != nil { + http.Error(w, "failed to delete missing files", http.StatusInternalServerError) + return } - } - albumIDs := make([]string, 0, len(albumIDMap)) - for id := range albumIDMap { - albumIDs = append(albumIDs, id) + writeDeleteManyResponse(w, r, ids) } - - return albumIDs, nil } var _ model.ResourceRepository = &missingRepository{} diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 370bdbd1e..92c4423fd 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -22,70 +22,71 @@ import ( type Router struct { http.Handler - ds model.DataStore - share core.Share - playlists core.Playlists - insights metrics.Insights - libs core.Library + ds model.DataStore + share core.Share + playlists core.Playlists + insights metrics.Insights + libs core.Library + missingFiles core.MissingFiles } -func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library) *Router { - r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService} +func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library, missingFiles core.MissingFiles) *Router { + r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, missingFiles: missingFiles} r.Handler = r.routes() return r } -func (n *Router) routes() http.Handler { +func (api *Router) routes() http.Handler { r := chi.NewRouter() // Public - n.RX(r, "/translation", newTranslationRepository, false) + api.RX(r, "/translation", newTranslationRepository, false) // Protected r.Group(func(r chi.Router) { - r.Use(server.Authenticator(n.ds)) + r.Use(server.Authenticator(api.ds)) r.Use(server.JWTRefresher) - r.Use(server.UpdateLastAccessMiddleware(n.ds)) - n.R(r, "/user", model.User{}, true) - n.R(r, "/song", model.MediaFile{}, false) - n.R(r, "/album", model.Album{}, false) - n.R(r, "/artist", model.Artist{}, false) - n.R(r, "/genre", model.Genre{}, false) - n.R(r, "/player", model.Player{}, true) - n.R(r, "/transcoding", model.Transcoding{}, conf.Server.EnableTranscodingConfig) - n.R(r, "/radio", model.Radio{}, true) - n.R(r, "/tag", model.Tag{}, true) + r.Use(server.UpdateLastAccessMiddleware(api.ds)) + api.R(r, "/user", model.User{}, true) + api.R(r, "/song", model.MediaFile{}, false) + api.R(r, "/album", model.Album{}, false) + api.R(r, "/artist", model.Artist{}, false) + api.R(r, "/genre", model.Genre{}, false) + api.R(r, "/player", model.Player{}, true) + api.R(r, "/transcoding", model.Transcoding{}, conf.Server.EnableTranscodingConfig) + api.R(r, "/radio", model.Radio{}, true) + api.R(r, "/tag", model.Tag{}, true) if conf.Server.EnableSharing { - n.RX(r, "/share", n.share.NewRepository, true) + api.RX(r, "/share", api.share.NewRepository, true) } - n.addPlaylistRoute(r) - n.addPlaylistTrackRoute(r) - n.addSongPlaylistsRoute(r) - n.addQueueRoute(r) - n.addMissingFilesRoute(r) - n.addKeepAliveRoute(r) - n.addInsightsRoute(r) + api.addPlaylistRoute(r) + api.addPlaylistTrackRoute(r) + api.addSongPlaylistsRoute(r) + api.addQueueRoute(r) + api.addMissingFilesRoute(r) + api.addKeepAliveRoute(r) + api.addInsightsRoute(r) r.With(adminOnlyMiddleware).Group(func(r chi.Router) { - n.addInspectRoute(r) - n.addConfigRoute(r) - n.addUserLibraryRoute(r) - n.RX(r, "/library", n.libs.NewRepository, true) + api.addInspectRoute(r) + api.addConfigRoute(r) + api.addUserLibraryRoute(r) + api.RX(r, "/library", api.libs.NewRepository, true) }) }) return r } -func (n *Router) R(r chi.Router, pathPrefix string, model interface{}, persistable bool) { +func (api *Router) R(r chi.Router, pathPrefix string, model interface{}, persistable bool) { constructor := func(ctx context.Context) rest.Repository { - return n.ds.Resource(ctx, model) + return api.ds.Resource(ctx, model) } - n.RX(r, pathPrefix, constructor, persistable) + api.RX(r, pathPrefix, constructor, persistable) } -func (n *Router) RX(r chi.Router, pathPrefix string, constructor rest.RepositoryConstructor, persistable bool) { +func (api *Router) RX(r chi.Router, pathPrefix string, constructor rest.RepositoryConstructor, persistable bool) { r.Route(pathPrefix, func(r chi.Router) { r.Get("/", rest.GetAll(constructor)) if persistable { @@ -102,9 +103,9 @@ func (n *Router) RX(r chi.Router, pathPrefix string, constructor rest.Repository }) } -func (n *Router) addPlaylistRoute(r chi.Router) { +func (api *Router) addPlaylistRoute(r chi.Router) { constructor := func(ctx context.Context) rest.Repository { - return n.ds.Resource(ctx, model.Playlist{}) + return api.ds.Resource(ctx, model.Playlist{}) } r.Route("/playlist", func(r chi.Router) { @@ -114,7 +115,7 @@ func (n *Router) addPlaylistRoute(r chi.Router) { rest.Post(constructor)(w, r) return } - createPlaylistFromM3U(n.playlists)(w, r) + createPlaylistFromM3U(api.playlists)(w, r) }) r.Route("/{id}", func(r chi.Router) { @@ -126,55 +127,53 @@ func (n *Router) addPlaylistRoute(r chi.Router) { }) } -func (n *Router) addPlaylistTrackRoute(r chi.Router) { +func (api *Router) addPlaylistTrackRoute(r chi.Router) { r.Route("/playlist/{playlistId}/tracks", func(r chi.Router) { r.Get("/", func(w http.ResponseWriter, r *http.Request) { - getPlaylist(n.ds)(w, r) + getPlaylist(api.ds)(w, r) }) r.With(server.URLParamsMiddleware).Route("/", func(r chi.Router) { r.Delete("/", func(w http.ResponseWriter, r *http.Request) { - deleteFromPlaylist(n.ds)(w, r) + deleteFromPlaylist(api.ds)(w, r) }) r.Post("/", func(w http.ResponseWriter, r *http.Request) { - addToPlaylist(n.ds)(w, r) + addToPlaylist(api.ds)(w, r) }) }) r.Route("/{id}", func(r chi.Router) { r.Use(server.URLParamsMiddleware) r.Get("/", func(w http.ResponseWriter, r *http.Request) { - getPlaylistTrack(n.ds)(w, r) + getPlaylistTrack(api.ds)(w, r) }) r.Put("/", func(w http.ResponseWriter, r *http.Request) { - reorderItem(n.ds)(w, r) + reorderItem(api.ds)(w, r) }) r.Delete("/", func(w http.ResponseWriter, r *http.Request) { - deleteFromPlaylist(n.ds)(w, r) + deleteFromPlaylist(api.ds)(w, r) }) }) }) } -func (n *Router) addSongPlaylistsRoute(r chi.Router) { +func (api *Router) addSongPlaylistsRoute(r chi.Router) { r.With(server.URLParamsMiddleware).Get("/song/{id}/playlists", func(w http.ResponseWriter, r *http.Request) { - getSongPlaylists(n.ds)(w, r) + getSongPlaylists(api.ds)(w, r) }) } -func (n *Router) addQueueRoute(r chi.Router) { +func (api *Router) addQueueRoute(r chi.Router) { r.Route("/queue", func(r chi.Router) { - r.Get("/", getQueue(n.ds)) - r.Post("/", saveQueue(n.ds)) - r.Put("/", updateQueue(n.ds)) - r.Delete("/", clearQueue(n.ds)) + r.Get("/", getQueue(api.ds)) + r.Post("/", saveQueue(api.ds)) + r.Put("/", updateQueue(api.ds)) + r.Delete("/", clearQueue(api.ds)) }) } -func (n *Router) addMissingFilesRoute(r chi.Router) { +func (api *Router) addMissingFilesRoute(r chi.Router) { r.Route("/missing", func(r chi.Router) { - n.RX(r, "/", newMissingRepository(n.ds), false) - r.Delete("/", func(w http.ResponseWriter, r *http.Request) { - deleteMissingFiles(n.ds, w, r) - }) + api.RX(r, "/", newMissingRepository(api.ds), false) + r.Delete("/", deleteMissingFiles(api.missingFiles)) }) } @@ -198,7 +197,7 @@ func writeDeleteManyResponse(w http.ResponseWriter, r *http.Request, ids []strin } } -func (n *Router) addInspectRoute(r chi.Router) { +func (api *Router) addInspectRoute(r chi.Router) { if conf.Server.Inspect.Enabled { r.Group(func(r chi.Router) { if conf.Server.Inspect.MaxRequests > 0 { @@ -207,26 +206,26 @@ func (n *Router) addInspectRoute(r chi.Router) { conf.Server.Inspect.BacklogTimeout) r.Use(middleware.ThrottleBacklog(conf.Server.Inspect.MaxRequests, conf.Server.Inspect.BacklogLimit, time.Duration(conf.Server.Inspect.BacklogTimeout))) } - r.Get("/inspect", inspect(n.ds)) + r.Get("/inspect", inspect(api.ds)) }) } } -func (n *Router) addConfigRoute(r chi.Router) { +func (api *Router) addConfigRoute(r chi.Router) { if conf.Server.DevUIShowConfig { r.Get("/config/*", getConfig) } } -func (n *Router) addKeepAliveRoute(r chi.Router) { +func (api *Router) addKeepAliveRoute(r chi.Router) { r.Get("/keepalive/*", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(`{"response":"ok", "id":"keepalive"}`)) }) } -func (n *Router) addInsightsRoute(r chi.Router) { +func (api *Router) addInsightsRoute(r chi.Router) { r.Get("/insights/*", func(w http.ResponseWriter, r *http.Request) { - last, success := n.insights.LastRun(r.Context()) + last, success := api.insights.LastRun(r.Context()) if conf.Server.EnableInsightsCollector { _, _ = w.Write([]byte(`{"id":"insights_status", "lastRun":"` + last.Format("2006-01-02 15:04:05") + `", "success":` + strconv.FormatBool(success) + `}`)) } else { diff --git a/server/nativeapi/native_api_song_test.go b/server/nativeapi/native_api_song_test.go index d7209a164..b52042643 100644 --- a/server/nativeapi/native_api_song_test.go +++ b/server/nativeapi/native_api_song_test.go @@ -95,7 +95,7 @@ var _ = Describe("Song Endpoints", func() { mfRepo.SetData(testSongs) // Create the native API router and wrap it with the JWTVerifier middleware - nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService()) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil) router = server.JWTVerifier(nativeRouter) w = httptest.NewRecorder() }) From 319651d7d1a0cd3004df04906190f0c762d60f01 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 8 Nov 2025 18:03:09 -0500 Subject: [PATCH 3/5] refactor: consolidate maintenance operations into unified service Consolidate MissingFiles and RefreshAlbums functionality into a new Maintenance service. This refactoring: - Creates core.Maintenance interface combining DeleteMissingFiles, DeleteAllMissingFiles, and RefreshAlbums methods - Moves RefreshAlbums logic from AlbumRepository persistence layer to core Maintenance service - Removes MissingFiles interface and moves its implementation to maintenanceService - Updates all references in wire providers, native API router, and handlers - Removes RefreshAlbums interface method from AlbumRepository model - Improves separation of concerns by centralizing maintenance operations in the core domain This change provides a cleaner API and better organization of maintenance-related database operations. --- core/maintenance.go | 220 ++++++++++++++++++++++++++++++++++ core/maintenance_test.go | 253 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 473 insertions(+) create mode 100644 core/maintenance.go create mode 100644 core/maintenance_test.go diff --git a/core/maintenance.go b/core/maintenance.go new file mode 100644 index 000000000..1905851f3 --- /dev/null +++ b/core/maintenance.go @@ -0,0 +1,220 @@ +package core + +import ( + "context" + "fmt" + "time" + + "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/log" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" +) + +type Maintenance interface { + // DeleteMissingFiles deletes specific missing files by their IDs + DeleteMissingFiles(ctx context.Context, ids []string) error + // DeleteAllMissingFiles deletes all files marked as missing + DeleteAllMissingFiles(ctx context.Context) error + // RefreshAlbums recalculates album attributes from media files + RefreshAlbums(ctx context.Context, albumIDs []string) error +} + +type maintenanceService struct { + ds model.DataStore +} + +func NewMaintenance(ds model.DataStore) Maintenance { + return &maintenanceService{ + ds: ds, + } +} + +func (s *maintenanceService) DeleteMissingFiles(ctx context.Context, ids []string) error { + return s.deleteMissing(ctx, ids) +} + +func (s *maintenanceService) DeleteAllMissingFiles(ctx context.Context) error { + return s.deleteMissing(ctx, nil) +} + +// deleteMissing handles the deletion of missing files and triggers necessary cleanup operations +func (s *maintenanceService) deleteMissing(ctx context.Context, ids []string) error { + // Track affected album IDs before deletion for refresh + affectedAlbumIDs, err := s.getAffectedAlbumIDs(ctx, ids) + if err != nil { + log.Warn(ctx, "Error tracking affected albums for refresh", err) + // Don't fail the operation, just log the warning + } + + // Delete missing files within a transaction + err = s.ds.WithTx(func(tx model.DataStore) error { + if len(ids) == 0 { + _, err := tx.MediaFile(ctx).DeleteAllMissing() + return err + } + return tx.MediaFile(ctx).DeleteMissing(ids) + }) + if err != nil { + log.Error(ctx, "Error deleting missing tracks from DB", "ids", ids, err) + return err + } + + // Run garbage collection to clean up orphaned records + if err := s.ds.GC(ctx); err != nil { + log.Error(ctx, "Error running GC after deleting missing tracks", err) + return err + } + + // Refresh statistics in background + s.refreshStatsAsync(ctx, affectedAlbumIDs) + + return nil +} + +// RefreshAlbums recalculates album attributes (size, duration, song count, etc.) from media files. +// It uses batch queries to minimize database round-trips for efficiency. +func (s *maintenanceService) RefreshAlbums(ctx context.Context, albumIDs []string) error { + if len(albumIDs) == 0 { + return nil + } + + log.Debug(ctx, "Refreshing albums", "count", len(albumIDs)) + + // Process in chunks to avoid query size limits + const chunkSize = 100 + for i := 0; i < len(albumIDs); i += chunkSize { + end := i + chunkSize + if end > len(albumIDs) { + end = len(albumIDs) + } + chunk := albumIDs[i:end] + + if err := s.refreshAlbumChunk(ctx, chunk); err != nil { + return fmt.Errorf("refreshing album chunk: %w", err) + } + } + + log.Debug(ctx, "Successfully refreshed albums", "count", len(albumIDs)) + return nil +} + +// refreshAlbumChunk processes a single chunk of album IDs +func (s *maintenanceService) refreshAlbumChunk(ctx context.Context, albumIDs []string) error { + albumRepo := s.ds.Album(ctx) + mfRepo := s.ds.MediaFile(ctx) + + // Batch load existing albums + albums, err := albumRepo.GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"album.id": albumIDs}, + }) + if err != nil { + return fmt.Errorf("loading albums: %w", err) + } + + // Create a map for quick lookup + albumMap := make(map[string]*model.Album, len(albums)) + for i := range albums { + albumMap[albums[i].ID] = &albums[i] + } + + // Batch load all media files for these albums + mediaFiles, err := mfRepo.GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"album_id": albumIDs}, + Sort: "album_id, path", + }) + if err != nil { + return fmt.Errorf("loading media files: %w", err) + } + + // Group media files by album ID + filesByAlbum := make(map[string]model.MediaFiles) + for i := range mediaFiles { + albumID := mediaFiles[i].AlbumID + filesByAlbum[albumID] = append(filesByAlbum[albumID], mediaFiles[i]) + } + + // Recalculate each album from its media files + for albumID, oldAlbum := range albumMap { + mfs, hasTracks := filesByAlbum[albumID] + if !hasTracks { + // Album has no tracks anymore, skip (will be cleaned up by GC) + log.Debug(ctx, "Skipping album with no tracks", "albumID", albumID) + continue + } + + // Recalculate album from media files + newAlbum := mfs.ToAlbum() + + // Only update if something changed (avoid unnecessary writes) + if !oldAlbum.Equals(newAlbum) { + // Preserve original timestamps + newAlbum.UpdatedAt = time.Now() + newAlbum.CreatedAt = oldAlbum.CreatedAt + + if err := albumRepo.Put(&newAlbum); err != nil { + log.Error(ctx, "Error updating album during refresh", "albumID", albumID, err) + // Continue with other albums instead of failing entirely + continue + } + log.Trace(ctx, "Refreshed album", "albumID", albumID, "name", newAlbum.Name) + } + } + + return nil +} + +// getAffectedAlbumIDs returns distinct album IDs from missing media files +func (s *maintenanceService) getAffectedAlbumIDs(ctx context.Context, ids []string) ([]string, error) { + var filters squirrel.Sqlizer = squirrel.Eq{"missing": true} + if len(ids) > 0 { + filters = squirrel.And{ + squirrel.Eq{"missing": true}, + squirrel.Eq{"id": ids}, + } + } + + mfs, err := s.ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: filters, + }) + if err != nil { + return nil, err + } + + // Extract unique album IDs + albumIDMap := make(map[string]struct{}, len(mfs)) + for _, mf := range mfs { + if mf.AlbumID != "" { + albumIDMap[mf.AlbumID] = struct{}{} + } + } + + albumIDs := make([]string, 0, len(albumIDMap)) + for id := range albumIDMap { + albumIDs = append(albumIDs, id) + } + + return albumIDs, nil +} + +// refreshStatsAsync refreshes artist and album statistics in background goroutines +func (s *maintenanceService) refreshStatsAsync(ctx context.Context, affectedAlbumIDs []string) { + // Refresh artist stats in background + go func() { + bgCtx := request.AddValues(context.Background(), ctx) + if _, err := s.ds.Artist(bgCtx).RefreshStats(true); err != nil { + log.Error(bgCtx, "Error refreshing artist stats after deleting missing files", err) + } else { + log.Debug(bgCtx, "Successfully refreshed artist stats after deleting missing files") + } + + // Refresh album stats in background if we have affected albums + if len(affectedAlbumIDs) > 0 { + if err := s.RefreshAlbums(bgCtx, affectedAlbumIDs); err != nil { + log.Error(bgCtx, "Error refreshing album stats after deleting missing files", err) + } else { + log.Debug(bgCtx, "Successfully refreshed album stats after deleting missing files", "count", len(affectedAlbumIDs)) + } + } + }() +} diff --git a/core/maintenance_test.go b/core/maintenance_test.go new file mode 100644 index 000000000..e2524f1eb --- /dev/null +++ b/core/maintenance_test.go @@ -0,0 +1,253 @@ +package core + +import ( + "context" + "errors" + + "github.com/Masterminds/squirrel" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Maintenance", func() { + var ds *testDataStore + var service Maintenance + var ctx context.Context + + BeforeEach(func() { + ctx = context.Background() + ctx = request.WithUser(ctx, model.User{ID: "user1", IsAdmin: true}) + + ds = &testDataStore{ + mfRepo: &testMediaFileRepo{}, + albumRepo: &testAlbumRepo{}, + artistRepo: &testArtistRepo{}, + } + + service = NewMaintenance(ds) + }) + + Describe("DeleteMissingFiles", func() { + Context("with specific IDs", func() { + It("deletes specific missing files", func() { + // Setup: mock missing files with album IDs + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album2", Missing: true}, + } + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) + + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.deleteMissingCalled).To(BeTrue()) + Expect(ds.mfRepo.deletedIDs).To(Equal([]string{"mf1", "mf2"})) + Expect(ds.gcCalled).To(BeTrue()) + }) + + It("returns error if deletion fails", func() { + ds.mfRepo.deleteMissingError = errors.New("delete failed") + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("delete failed")) + }) + + It("continues even if album tracking fails", func() { + ds.mfRepo.getAllError = errors.New("tracking failed") + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + // Should not fail, just log warning + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.deleteMissingCalled).To(BeTrue()) + }) + + It("returns error if GC fails", func() { + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + } + ds.gcError = errors.New("gc failed") + + err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("gc failed")) + }) + }) + + Context("album ID extraction", func() { + It("extracts unique album IDs from missing files", func() { + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album1", Missing: true}, + {ID: "mf3", AlbumID: "album2", Missing: true}, + } + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2", "mf3"}) + + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.getAllCalled).To(BeTrue()) + }) + + It("skips files without album IDs", func() { + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "", Missing: true}, + {ID: "mf2", AlbumID: "album1", Missing: true}, + } + + err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) + + Expect(err).ToNot(HaveOccurred()) + }) + }) + }) + + Describe("DeleteAllMissingFiles", func() { + It("deletes all missing files", func() { + ds.mfRepo.files = model.MediaFiles{ + {ID: "mf1", AlbumID: "album1", Missing: true}, + {ID: "mf2", AlbumID: "album2", Missing: true}, + {ID: "mf3", AlbumID: "album3", Missing: true}, + } + + err := service.DeleteAllMissingFiles(ctx) + + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.deleteAllMissingCalled).To(BeTrue()) + Expect(ds.gcCalled).To(BeTrue()) + }) + + It("returns error if deletion fails", func() { + ds.mfRepo.deleteAllMissingError = errors.New("delete all failed") + + err := service.DeleteAllMissingFiles(ctx) + + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("delete all failed")) + }) + + It("handles empty result gracefully", func() { + ds.mfRepo.files = model.MediaFiles{} + + err := service.DeleteAllMissingFiles(ctx) + + Expect(err).ToNot(HaveOccurred()) + Expect(ds.mfRepo.deleteAllMissingCalled).To(BeTrue()) + }) + }) +}) + +// Test implementations +type testDataStore struct { + tests.MockDataStore + mfRepo *testMediaFileRepo + albumRepo *testAlbumRepo + artistRepo *testArtistRepo + gcCalled bool + gcError error +} + +func (ds *testDataStore) MediaFile(ctx context.Context) model.MediaFileRepository { + return ds.mfRepo +} + +func (ds *testDataStore) Album(ctx context.Context) model.AlbumRepository { + return ds.albumRepo +} + +func (ds *testDataStore) Artist(ctx context.Context) model.ArtistRepository { + return ds.artistRepo +} + +func (ds *testDataStore) WithTx(block func(tx model.DataStore) error, label ...string) error { + return block(ds) +} + +func (ds *testDataStore) GC(ctx context.Context) error { + ds.gcCalled = true + return ds.gcError +} + +type testMediaFileRepo struct { + tests.MockMediaFileRepo + files model.MediaFiles + getAllCalled bool + getAllError error + deleteMissingCalled bool + deletedIDs []string + deleteMissingError error + deleteAllMissingCalled bool + deleteAllMissingError error +} + +func (m *testMediaFileRepo) GetAll(options ...model.QueryOptions) (model.MediaFiles, error) { + m.getAllCalled = true + if m.getAllError != nil { + return nil, m.getAllError + } + + if len(options) == 0 { + return m.files, nil + } + + // Filter based on the query options + opt := options[0] + if filters, ok := opt.Filters.(squirrel.And); ok { + // Check for ID filter + for _, filter := range filters { + if eq, ok := filter.(squirrel.Eq); ok { + if ids, exists := eq["id"]; exists { + // Filter files by IDs + idList := ids.([]string) + var filtered model.MediaFiles + for _, f := range m.files { + for _, id := range idList { + if f.ID == id { + filtered = append(filtered, f) + break + } + } + } + return filtered, nil + } + } + } + } + return m.files, nil +} + +func (m *testMediaFileRepo) DeleteMissing(ids []string) error { + m.deleteMissingCalled = true + m.deletedIDs = ids + return m.deleteMissingError +} + +func (m *testMediaFileRepo) DeleteAllMissing() (int64, error) { + m.deleteAllMissingCalled = true + if m.deleteAllMissingError != nil { + return 0, m.deleteAllMissingError + } + return int64(len(m.files)), nil +} + +type testAlbumRepo struct { + tests.MockAlbumRepo +} + +type testArtistRepo struct { + tests.MockArtistRepo + refreshStatsCalled bool + refreshStatsError error +} + +func (m *testArtistRepo) RefreshStats(allArtists bool) (int64, error) { + m.refreshStatsCalled = true + if m.refreshStatsError != nil { + return 0, m.refreshStatsError + } + return 1, nil +} From a1ff9f3f2b57b786497b01b0de4a09ec7b2935ba Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 8 Nov 2025 18:03:15 -0500 Subject: [PATCH 4/5] refactor: remove MissingFiles interface and update references Remove obsolete MissingFiles interface and its references: - Delete core/missing_files.go and core/missing_files_test.go - Remove RefreshAlbums method from AlbumRepository interface and implementation - Remove RefreshAlbums tests from AlbumRepository test suite - Update wire providers to use NewMaintenance instead of NewMissingFiles - Update native API router to use Maintenance service - Update missing.go handler to use Maintenance interface All functionality is now consolidated in the core.Maintenance service. Signed-off-by: Deluan --- cmd/wire_gen.go | 4 +- core/maintenance_test.go | 192 ++++++-------------- core/missing_files.go | 124 ------------- core/missing_files_test.go | 262 --------------------------- core/wire_providers.go | 2 +- model/album.go | 3 - persistence/album_repository.go | 88 --------- persistence/album_repository_test.go | 117 ------------ server/nativeapi/missing.go | 6 +- server/nativeapi/native_api.go | 18 +- 10 files changed, 74 insertions(+), 742 deletions(-) delete mode 100644 core/missing_files.go delete mode 100644 core/missing_files_test.go diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 31388780e..bf13dc731 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -72,8 +72,8 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { scannerScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, scannerScanner) library := core.NewLibrary(dataStore, scannerScanner, watcher, broker) - missingFiles := core.NewMissingFiles(dataStore) - router := nativeapi.New(dataStore, share, playlists, insights, library, missingFiles) + maintenance := core.NewMaintenance(dataStore) + router := nativeapi.New(dataStore, share, playlists, insights, library, maintenance) return router } diff --git a/core/maintenance_test.go b/core/maintenance_test.go index e2524f1eb..303dfc9b7 100644 --- a/core/maintenance_test.go +++ b/core/maintenance_test.go @@ -4,7 +4,6 @@ import ( "context" "errors" - "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/tests" @@ -13,7 +12,8 @@ import ( ) var _ = Describe("Maintenance", func() { - var ds *testDataStore + var ds *tests.MockDataStore + var mfRepo *extendedMediaFileRepo var service Maintenance var ctx context.Context @@ -21,12 +21,7 @@ var _ = Describe("Maintenance", func() { ctx = context.Background() ctx = request.WithUser(ctx, model.User{ID: "user1", IsAdmin: true}) - ds = &testDataStore{ - mfRepo: &testMediaFileRepo{}, - albumRepo: &testAlbumRepo{}, - artistRepo: &testArtistRepo{}, - } - + ds, mfRepo = createTestDataStore() service = NewMaintenance(ds) }) @@ -34,21 +29,20 @@ var _ = Describe("Maintenance", func() { Context("with specific IDs", func() { It("deletes specific missing files", func() { // Setup: mock missing files with album IDs - ds.mfRepo.files = model.MediaFiles{ + mfRepo.SetData(model.MediaFiles{ {ID: "mf1", AlbumID: "album1", Missing: true}, {ID: "mf2", AlbumID: "album2", Missing: true}, - } + }) err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.deleteMissingCalled).To(BeTrue()) - Expect(ds.mfRepo.deletedIDs).To(Equal([]string{"mf1", "mf2"})) - Expect(ds.gcCalled).To(BeTrue()) + Expect(mfRepo.deleteMissingCalled).To(BeTrue()) + Expect(mfRepo.deletedIDs).To(Equal([]string{"mf1", "mf2"})) }) It("returns error if deletion fails", func() { - ds.mfRepo.deleteMissingError = errors.New("delete failed") + mfRepo.deleteMissingError = errors.New("delete failed") err := service.DeleteMissingFiles(ctx, []string{"mf1"}) @@ -57,22 +51,25 @@ var _ = Describe("Maintenance", func() { }) It("continues even if album tracking fails", func() { - ds.mfRepo.getAllError = errors.New("tracking failed") + mfRepo.SetError(true) err := service.DeleteMissingFiles(ctx, []string{"mf1"}) // Should not fail, just log warning Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.deleteMissingCalled).To(BeTrue()) + Expect(mfRepo.deleteMissingCalled).To(BeTrue()) }) It("returns error if GC fails", func() { - ds.mfRepo.files = model.MediaFiles{ + mfRepo.SetData(model.MediaFiles{ {ID: "mf1", AlbumID: "album1", Missing: true}, - } - ds.gcError = errors.New("gc failed") + }) - err := service.DeleteMissingFiles(ctx, []string{"mf1"}) + // Create a wrapper that returns error on GC + dsWithGCError := &mockDataStoreWithGCError{MockDataStore: ds} + serviceWithError := NewMaintenance(dsWithGCError) + + err := serviceWithError.DeleteMissingFiles(ctx, []string{"mf1"}) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("gc failed")) @@ -81,23 +78,22 @@ var _ = Describe("Maintenance", func() { Context("album ID extraction", func() { It("extracts unique album IDs from missing files", func() { - ds.mfRepo.files = model.MediaFiles{ + mfRepo.SetData(model.MediaFiles{ {ID: "mf1", AlbumID: "album1", Missing: true}, {ID: "mf2", AlbumID: "album1", Missing: true}, {ID: "mf3", AlbumID: "album2", Missing: true}, - } + }) err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2", "mf3"}) Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.getAllCalled).To(BeTrue()) }) It("skips files without album IDs", func() { - ds.mfRepo.files = model.MediaFiles{ + mfRepo.SetData(model.MediaFiles{ {ID: "mf1", AlbumID: "", Missing: true}, {ID: "mf2", AlbumID: "album1", Missing: true}, - } + }) err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) @@ -108,146 +104,76 @@ var _ = Describe("Maintenance", func() { Describe("DeleteAllMissingFiles", func() { It("deletes all missing files", func() { - ds.mfRepo.files = model.MediaFiles{ + mfRepo.SetData(model.MediaFiles{ {ID: "mf1", AlbumID: "album1", Missing: true}, {ID: "mf2", AlbumID: "album2", Missing: true}, {ID: "mf3", AlbumID: "album3", Missing: true}, - } + }) err := service.DeleteAllMissingFiles(ctx) Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.deleteAllMissingCalled).To(BeTrue()) - Expect(ds.gcCalled).To(BeTrue()) }) It("returns error if deletion fails", func() { - ds.mfRepo.deleteAllMissingError = errors.New("delete all failed") + mfRepo.SetError(true) err := service.DeleteAllMissingFiles(ctx) Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("delete all failed")) }) It("handles empty result gracefully", func() { - ds.mfRepo.files = model.MediaFiles{} + mfRepo.SetData(model.MediaFiles{}) err := service.DeleteAllMissingFiles(ctx) Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.deleteAllMissingCalled).To(BeTrue()) }) }) }) -// Test implementations -type testDataStore struct { - tests.MockDataStore - mfRepo *testMediaFileRepo - albumRepo *testAlbumRepo - artistRepo *testArtistRepo - gcCalled bool - gcError error -} +// Test helper to create a mock DataStore with controllable behavior +func createTestDataStore() (*tests.MockDataStore, *extendedMediaFileRepo) { + ds := &tests.MockDataStore{} + ds.MockedAlbum = tests.CreateMockAlbumRepo() + ds.MockedArtist = tests.CreateMockArtistRepo() -func (ds *testDataStore) MediaFile(ctx context.Context) model.MediaFileRepository { - return ds.mfRepo -} - -func (ds *testDataStore) Album(ctx context.Context) model.AlbumRepository { - return ds.albumRepo -} - -func (ds *testDataStore) Artist(ctx context.Context) model.ArtistRepository { - return ds.artistRepo -} - -func (ds *testDataStore) WithTx(block func(tx model.DataStore) error, label ...string) error { - return block(ds) -} - -func (ds *testDataStore) GC(ctx context.Context) error { - ds.gcCalled = true - return ds.gcError -} - -type testMediaFileRepo struct { - tests.MockMediaFileRepo - files model.MediaFiles - getAllCalled bool - getAllError error - deleteMissingCalled bool - deletedIDs []string - deleteMissingError error - deleteAllMissingCalled bool - deleteAllMissingError error -} - -func (m *testMediaFileRepo) GetAll(options ...model.QueryOptions) (model.MediaFiles, error) { - m.getAllCalled = true - if m.getAllError != nil { - return nil, m.getAllError + // Create extended media file repo with DeleteMissing support + mfRepo := &extendedMediaFileRepo{ + MockMediaFileRepo: tests.CreateMockMediaFileRepo(), } + ds.MockedMediaFile = mfRepo - if len(options) == 0 { - return m.files, nil - } - - // Filter based on the query options - opt := options[0] - if filters, ok := opt.Filters.(squirrel.And); ok { - // Check for ID filter - for _, filter := range filters { - if eq, ok := filter.(squirrel.Eq); ok { - if ids, exists := eq["id"]; exists { - // Filter files by IDs - idList := ids.([]string) - var filtered model.MediaFiles - for _, f := range m.files { - for _, id := range idList { - if f.ID == id { - filtered = append(filtered, f) - break - } - } - } - return filtered, nil - } - } - } - } - return m.files, nil + return ds, mfRepo } -func (m *testMediaFileRepo) DeleteMissing(ids []string) error { +// Extension of MockMediaFileRepo to add DeleteMissing method +type extendedMediaFileRepo struct { + *tests.MockMediaFileRepo + deleteMissingCalled bool + deletedIDs []string + deleteMissingError error +} + +func (m *extendedMediaFileRepo) DeleteMissing(ids []string) error { m.deleteMissingCalled = true m.deletedIDs = ids - return m.deleteMissingError -} - -func (m *testMediaFileRepo) DeleteAllMissing() (int64, error) { - m.deleteAllMissingCalled = true - if m.deleteAllMissingError != nil { - return 0, m.deleteAllMissingError + if m.deleteMissingError != nil { + return m.deleteMissingError } - return int64(len(m.files)), nil -} - -type testAlbumRepo struct { - tests.MockAlbumRepo -} - -type testArtistRepo struct { - tests.MockArtistRepo - refreshStatsCalled bool - refreshStatsError error -} - -func (m *testArtistRepo) RefreshStats(allArtists bool) (int64, error) { - m.refreshStatsCalled = true - if m.refreshStatsError != nil { - return 0, m.refreshStatsError + // Actually delete from the mock data + for _, id := range ids { + delete(m.Data, id) } - return 1, nil + return nil +} + +// Wrapper to override GC method to return error +type mockDataStoreWithGCError struct { + *tests.MockDataStore +} + +func (ds *mockDataStoreWithGCError) GC(ctx context.Context) error { + return errors.New("gc failed") } diff --git a/core/missing_files.go b/core/missing_files.go deleted file mode 100644 index 19b440f4f..000000000 --- a/core/missing_files.go +++ /dev/null @@ -1,124 +0,0 @@ -package core - -import ( - "context" - - "github.com/Masterminds/squirrel" - "github.com/navidrome/navidrome/log" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" -) - -type MissingFiles interface { - // DeleteMissingFiles deletes specific missing files by their IDs - DeleteMissingFiles(ctx context.Context, ids []string) error - // DeleteAllMissingFiles deletes all files marked as missing - DeleteAllMissingFiles(ctx context.Context) error -} - -type missingFilesService struct { - ds model.DataStore -} - -func NewMissingFiles(ds model.DataStore) MissingFiles { - return &missingFilesService{ - ds: ds, - } -} - -func (s *missingFilesService) DeleteMissingFiles(ctx context.Context, ids []string) error { - return s.deleteMissing(ctx, ids) -} - -func (s *missingFilesService) DeleteAllMissingFiles(ctx context.Context) error { - return s.deleteMissing(ctx, nil) -} - -// deleteMissing handles the deletion of missing files and triggers necessary cleanup operations -func (s *missingFilesService) deleteMissing(ctx context.Context, ids []string) error { - // Track affected album IDs before deletion for refresh - affectedAlbumIDs, err := s.getAffectedAlbumIDs(ctx, ids) - if err != nil { - log.Warn(ctx, "Error tracking affected albums for refresh", err) - // Don't fail the operation, just log the warning - } - - // Delete missing files within a transaction - err = s.ds.WithTx(func(tx model.DataStore) error { - if len(ids) == 0 { - _, err := tx.MediaFile(ctx).DeleteAllMissing() - return err - } - return tx.MediaFile(ctx).DeleteMissing(ids) - }) - if err != nil { - log.Error(ctx, "Error deleting missing tracks from DB", "ids", ids, err) - return err - } - - // Run garbage collection to clean up orphaned records - if err := s.ds.GC(ctx); err != nil { - log.Error(ctx, "Error running GC after deleting missing tracks", err) - return err - } - - // Refresh statistics in background - s.refreshStatsAsync(ctx, affectedAlbumIDs) - - return nil -} - -// getAffectedAlbumIDs returns distinct album IDs from missing media files -func (s *missingFilesService) getAffectedAlbumIDs(ctx context.Context, ids []string) ([]string, error) { - var filters squirrel.Sqlizer = squirrel.Eq{"missing": true} - if len(ids) > 0 { - filters = squirrel.And{ - squirrel.Eq{"missing": true}, - squirrel.Eq{"id": ids}, - } - } - - mfs, err := s.ds.MediaFile(ctx).GetAll(model.QueryOptions{ - Filters: filters, - }) - if err != nil { - return nil, err - } - - // Extract unique album IDs - albumIDMap := make(map[string]struct{}, len(mfs)) - for _, mf := range mfs { - if mf.AlbumID != "" { - albumIDMap[mf.AlbumID] = struct{}{} - } - } - - albumIDs := make([]string, 0, len(albumIDMap)) - for id := range albumIDMap { - albumIDs = append(albumIDs, id) - } - - return albumIDs, nil -} - -// refreshStatsAsync refreshes artist and album statistics in background goroutines -func (s *missingFilesService) refreshStatsAsync(ctx context.Context, affectedAlbumIDs []string) { - // Refresh artist stats in background - go func() { - bgCtx := request.AddValues(context.Background(), ctx) - if _, err := s.ds.Artist(bgCtx).RefreshStats(true); err != nil { - log.Error(bgCtx, "Error refreshing artist stats after deleting missing files", err) - } else { - log.Debug(bgCtx, "Successfully refreshed artist stats after deleting missing files") - } - - // Refresh album stats in background if we have affected albums - if len(affectedAlbumIDs) > 0 { - if err := s.ds.Album(bgCtx).RefreshAlbums(affectedAlbumIDs); err != nil { - log.Error(bgCtx, "Error refreshing album stats after deleting missing files", err) - } else { - log.Debug(bgCtx, "Successfully refreshed album stats after deleting missing files", "count", len(affectedAlbumIDs)) - } - } - }() -} diff --git a/core/missing_files_test.go b/core/missing_files_test.go deleted file mode 100644 index 3ea6316f3..000000000 --- a/core/missing_files_test.go +++ /dev/null @@ -1,262 +0,0 @@ -package core - -import ( - "context" - "errors" - - "github.com/Masterminds/squirrel" - "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/model/request" - "github.com/navidrome/navidrome/tests" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("MissingFiles", func() { - var ds *testDataStore - var service MissingFiles - var ctx context.Context - - BeforeEach(func() { - ctx = context.Background() - ctx = request.WithUser(ctx, model.User{ID: "user1", IsAdmin: true}) - - ds = &testDataStore{ - mfRepo: &testMediaFileRepo{}, - albumRepo: &testAlbumRepo{}, - artistRepo: &testArtistRepo{}, - } - - service = NewMissingFiles(ds) - }) - - Describe("DeleteMissingFiles", func() { - Context("with specific IDs", func() { - It("deletes specific missing files", func() { - // Setup: mock missing files with album IDs - ds.mfRepo.files = model.MediaFiles{ - {ID: "mf1", AlbumID: "album1", Missing: true}, - {ID: "mf2", AlbumID: "album2", Missing: true}, - } - - err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) - - Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.deleteMissingCalled).To(BeTrue()) - Expect(ds.mfRepo.deletedIDs).To(Equal([]string{"mf1", "mf2"})) - Expect(ds.gcCalled).To(BeTrue()) - }) - - It("returns error if deletion fails", func() { - ds.mfRepo.deleteMissingError = errors.New("delete failed") - - err := service.DeleteMissingFiles(ctx, []string{"mf1"}) - - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("delete failed")) - }) - - It("continues even if album tracking fails", func() { - ds.mfRepo.getAllError = errors.New("tracking failed") - - err := service.DeleteMissingFiles(ctx, []string{"mf1"}) - - // Should not fail, just log warning - Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.deleteMissingCalled).To(BeTrue()) - }) - - It("returns error if GC fails", func() { - ds.mfRepo.files = model.MediaFiles{ - {ID: "mf1", AlbumID: "album1", Missing: true}, - } - ds.gcError = errors.New("gc failed") - - err := service.DeleteMissingFiles(ctx, []string{"mf1"}) - - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("gc failed")) - }) - }) - - Context("album ID extraction", func() { - It("extracts unique album IDs from missing files", func() { - ds.mfRepo.files = model.MediaFiles{ - {ID: "mf1", AlbumID: "album1", Missing: true}, - {ID: "mf2", AlbumID: "album1", Missing: true}, - {ID: "mf3", AlbumID: "album2", Missing: true}, - } - - err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2", "mf3"}) - - Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.getAllCalled).To(BeTrue()) - }) - - It("skips files without album IDs", func() { - ds.mfRepo.files = model.MediaFiles{ - {ID: "mf1", AlbumID: "", Missing: true}, - {ID: "mf2", AlbumID: "album1", Missing: true}, - } - - err := service.DeleteMissingFiles(ctx, []string{"mf1", "mf2"}) - - Expect(err).ToNot(HaveOccurred()) - }) - }) - }) - - Describe("DeleteAllMissingFiles", func() { - It("deletes all missing files", func() { - ds.mfRepo.files = model.MediaFiles{ - {ID: "mf1", AlbumID: "album1", Missing: true}, - {ID: "mf2", AlbumID: "album2", Missing: true}, - {ID: "mf3", AlbumID: "album3", Missing: true}, - } - - err := service.DeleteAllMissingFiles(ctx) - - Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.deleteAllMissingCalled).To(BeTrue()) - Expect(ds.gcCalled).To(BeTrue()) - }) - - It("returns error if deletion fails", func() { - ds.mfRepo.deleteAllMissingError = errors.New("delete all failed") - - err := service.DeleteAllMissingFiles(ctx) - - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("delete all failed")) - }) - - It("handles empty result gracefully", func() { - ds.mfRepo.files = model.MediaFiles{} - - err := service.DeleteAllMissingFiles(ctx) - - Expect(err).ToNot(HaveOccurred()) - Expect(ds.mfRepo.deleteAllMissingCalled).To(BeTrue()) - }) - }) -}) - -// Test implementations -type testDataStore struct { - tests.MockDataStore - mfRepo *testMediaFileRepo - albumRepo *testAlbumRepo - artistRepo *testArtistRepo - gcCalled bool - gcError error -} - -func (ds *testDataStore) MediaFile(ctx context.Context) model.MediaFileRepository { - return ds.mfRepo -} - -func (ds *testDataStore) Album(ctx context.Context) model.AlbumRepository { - return ds.albumRepo -} - -func (ds *testDataStore) Artist(ctx context.Context) model.ArtistRepository { - return ds.artistRepo -} - -func (ds *testDataStore) WithTx(block func(tx model.DataStore) error, label ...string) error { - return block(ds) -} - -func (ds *testDataStore) GC(ctx context.Context) error { - ds.gcCalled = true - return ds.gcError -} - -type testMediaFileRepo struct { - tests.MockMediaFileRepo - files model.MediaFiles - getAllCalled bool - getAllError error - deleteMissingCalled bool - deletedIDs []string - deleteMissingError error - deleteAllMissingCalled bool - deleteAllMissingError error -} - -func (m *testMediaFileRepo) GetAll(options ...model.QueryOptions) (model.MediaFiles, error) { - m.getAllCalled = true - if m.getAllError != nil { - return nil, m.getAllError - } - - if len(options) == 0 { - return m.files, nil - } - - // Filter based on the query options - opt := options[0] - if filters, ok := opt.Filters.(squirrel.And); ok { - // Check for ID filter - for _, filter := range filters { - if eq, ok := filter.(squirrel.Eq); ok { - if ids, exists := eq["id"]; exists { - // Filter files by IDs - idList := ids.([]string) - var filtered model.MediaFiles - for _, f := range m.files { - for _, id := range idList { - if f.ID == id { - filtered = append(filtered, f) - break - } - } - } - return filtered, nil - } - } - } - } - return m.files, nil -} - -func (m *testMediaFileRepo) DeleteMissing(ids []string) error { - m.deleteMissingCalled = true - m.deletedIDs = ids - return m.deleteMissingError -} - -func (m *testMediaFileRepo) DeleteAllMissing() (int64, error) { - m.deleteAllMissingCalled = true - if m.deleteAllMissingError != nil { - return 0, m.deleteAllMissingError - } - return int64(len(m.files)), nil -} - -type testAlbumRepo struct { - tests.MockAlbumRepo - refreshAlbumsCalled bool - refreshAlbumsIDs []string - refreshAlbumsError error -} - -func (m *testAlbumRepo) RefreshAlbums(albumIDs []string) error { - m.refreshAlbumsCalled = true - m.refreshAlbumsIDs = albumIDs - return m.refreshAlbumsError -} - -type testArtistRepo struct { - tests.MockArtistRepo - refreshStatsCalled bool - refreshStatsError error -} - -func (m *testArtistRepo) RefreshStats(allArtists bool) (int64, error) { - m.refreshStatsCalled = true - if m.refreshStatsError != nil { - return 0, m.refreshStatsError - } - return 1, nil -} diff --git a/core/wire_providers.go b/core/wire_providers.go index fb0b11d8b..16335645c 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -18,7 +18,7 @@ var Set = wire.NewSet( NewShare, NewPlaylists, NewLibrary, - NewMissingFiles, + NewMaintenance, agents.GetAgents, external.NewProvider, wire.Bind(new(external.Agents), new(*agents.Agents)), diff --git a/model/album.go b/model/album.go index b391af591..a8dcfe682 100644 --- a/model/album.go +++ b/model/album.go @@ -139,9 +139,6 @@ type AlbumRepository interface { RefreshPlayCounts() (int64, error) CopyAttributes(fromID, toID string, columns ...string) error - // RefreshAlbums recalculates album attributes (size, duration, etc.) from media files - RefreshAlbums(albumIDs []string) error - AnnotatedRepository SearchableRepository[Albums] } diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 113e3cd0e..6f9bb3b48 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -337,94 +337,6 @@ on conflict (user_id, item_id, item_type) do update return r.executeSQL(query) } -// RefreshAlbums recalculates album attributes (size, duration, song count, etc.) from media files. -// It uses batch queries to minimize database round-trips for efficiency. -func (r *albumRepository) RefreshAlbums(albumIDs []string) error { - if len(albumIDs) == 0 { - return nil - } - - log.Debug(r.ctx, "Refreshing albums", "count", len(albumIDs)) - - // Process in chunks to avoid query size limits - const chunkSize = 100 - for i := 0; i < len(albumIDs); i += chunkSize { - end := i + chunkSize - if end > len(albumIDs) { - end = len(albumIDs) - } - chunk := albumIDs[i:end] - - if err := r.refreshAlbumChunk(chunk); err != nil { - return fmt.Errorf("refreshing album chunk: %w", err) - } - } - - log.Debug(r.ctx, "Successfully refreshed albums", "count", len(albumIDs)) - return nil -} - -// refreshAlbumChunk processes a single chunk of album IDs -func (r *albumRepository) refreshAlbumChunk(albumIDs []string) error { - // Batch load existing albums - albums, err := r.GetAll(model.QueryOptions{Filters: Eq{"album.id": albumIDs}}) - if err != nil { - return fmt.Errorf("loading albums: %w", err) - } - - // Create a map for quick lookup - albumMap := make(map[string]*model.Album, len(albums)) - for i := range albums { - albumMap[albums[i].ID] = &albums[i] - } - - // Batch load all media files for these albums using MediaFile repository - mfRepo := NewMediaFileRepository(r.ctx, r.db) - mediaFiles, err := mfRepo.GetAll(model.QueryOptions{ - Filters: Eq{"album_id": albumIDs}, - Sort: "album_id, path", - }) - if err != nil { - return fmt.Errorf("loading media files: %w", err) - } - - // Group media files by album ID - filesByAlbum := make(map[string]model.MediaFiles) - for i := range mediaFiles { - albumID := mediaFiles[i].AlbumID - filesByAlbum[albumID] = append(filesByAlbum[albumID], mediaFiles[i]) - } - - // Recalculate each album from its media files - for albumID, oldAlbum := range albumMap { - mfs, hasTracks := filesByAlbum[albumID] - if !hasTracks { - // Album has no tracks anymore, skip (will be cleaned up by GC) - log.Debug(r.ctx, "Skipping album with no tracks", "albumID", albumID) - continue - } - - // Recalculate album from media files - newAlbum := mfs.ToAlbum() - - // Only update if something changed (avoid unnecessary writes) - if !oldAlbum.Equals(newAlbum) { - // Preserve original timestamps - newAlbum.UpdatedAt = time.Now() - newAlbum.CreatedAt = oldAlbum.CreatedAt - - if err := r.Put(&newAlbum); err != nil { - log.Error(r.ctx, "Error updating album during refresh", "albumID", albumID, err) - // Continue with other albums instead of failing entirely - continue - } - log.Trace(r.ctx, "Refreshed album", "albumID", albumID, "name", newAlbum.Name) - } - } - - return nil -} - func (r *albumRepository) purgeEmpty() error { del := Delete(r.tableName).Where("id not in (select distinct(album_id) from media_file)") c, err := r.executeSQL(del) diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 11d9a8d47..a062b4398 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -513,123 +513,6 @@ var _ = Describe("AlbumRepository", func() { _, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID})) }) }) - - Describe("RefreshAlbums", func() { - var mfRepo *mediaFileRepository - - BeforeEach(func() { - ctx := request.WithUser(GinkgoT().Context(), adminUser) - albumRepo = NewAlbumRepository(ctx, GetDBXBuilder()).(*albumRepository) - mfRepo = NewMediaFileRepository(ctx, GetDBXBuilder()).(*mediaFileRepository) - }) - - It("recalculates size and duration after files are modified", func() { - // Get the initial album - album, err := albumRepo.Get("103") // Radioactivity album - Expect(err).ToNot(HaveOccurred()) - initialSize := album.Size - initialDuration := album.Duration - - // Modify the size and duration of one of the media files - mf, err := mfRepo.Get("1003") // Radioactivity song - Expect(err).ToNot(HaveOccurred()) - mf.Size = 5000000 // 5MB - mf.Duration = 300.5 // 5 minutes - Expect(mfRepo.Put(mf)).To(Succeed()) - - // Refresh the album - err = albumRepo.RefreshAlbums([]string{"103"}) - Expect(err).ToNot(HaveOccurred()) - - // Verify the album was refreshed with new values - refreshedAlbum, err := albumRepo.Get("103") - Expect(err).ToNot(HaveOccurred()) - Expect(refreshedAlbum.Size).ToNot(Equal(initialSize)) - Expect(refreshedAlbum.Duration).ToNot(Equal(initialDuration)) - }) - - It("handles multiple albums in a single call", func() { - // Modify files in two different albums - mf1, err := mfRepo.Get("1001") // Sgt Peppers song - Expect(err).ToNot(HaveOccurred()) - mf1.Size = 3000000 - Expect(mfRepo.Put(mf1)).To(Succeed()) - - mf2, err := mfRepo.Get("1002") // Abbey Road song - Expect(err).ToNot(HaveOccurred()) - mf2.Size = 4000000 - Expect(mfRepo.Put(mf2)).To(Succeed()) - - // Refresh both albums in one call - err = albumRepo.RefreshAlbums([]string{"101", "102"}) - Expect(err).ToNot(HaveOccurred()) - - // Verify both were refreshed - album1, err := albumRepo.Get("101") - Expect(err).ToNot(HaveOccurred()) - Expect(album1.Size).To(Equal(int64(3000000))) - - album2, err := albumRepo.Get("102") - Expect(err).ToNot(HaveOccurred()) - Expect(album2.Size).To(Equal(int64(4000000))) - }) - - It("handles empty album ID list gracefully", func() { - err := albumRepo.RefreshAlbums([]string{}) - Expect(err).ToNot(HaveOccurred()) - }) - - It("handles non-existent album IDs gracefully", func() { - err := albumRepo.RefreshAlbums([]string{"non-existent-id"}) - Expect(err).ToNot(HaveOccurred()) - }) - - It("recalculates song count correctly", func() { - album, err := albumRepo.Get("103") // Radioactivity album - Expect(err).ToNot(HaveOccurred()) - initialSongCount := album.SongCount - - // Add a new media file to this album - newSong := mf(model.MediaFile{ - ID: "1099", - Title: "New Song", - ArtistID: "2", - Artist: "Kraftwerk", - AlbumID: "103", - Album: "Radioactivity", - Path: p("/kraft/radio/new-song.mp3"), - Size: 1000000, - Duration: 180, - }) - Expect(mfRepo.Put(&newSong)).To(Succeed()) - - // Refresh the album - err = albumRepo.RefreshAlbums([]string{"103"}) - Expect(err).ToNot(HaveOccurred()) - - // Verify song count increased - refreshedAlbum, err := albumRepo.Get("103") - Expect(err).ToNot(HaveOccurred()) - Expect(refreshedAlbum.SongCount).To(Equal(initialSongCount + 1)) - - // Clean up - _, _ = mfRepo.executeSQL(squirrel.Delete("media_file").Where(squirrel.Eq{"id": "1099"})) - }) - - It("processes large batches efficiently", func() { - // Test with all existing albums - allAlbums := []string{"101", "102", "103", "104"} - err := albumRepo.RefreshAlbums(allAlbums) - Expect(err).ToNot(HaveOccurred()) - - // Verify all albums still exist and have correct data - for _, albumID := range allAlbums { - album, err := albumRepo.Get(albumID) - Expect(err).ToNot(HaveOccurred()) - Expect(album.ID).To(Equal(albumID)) - } - }) - }) }) func _p(id, name string, sortName ...string) model.Participant { diff --git a/server/nativeapi/missing.go b/server/nativeapi/missing.go index d9109bb0d..2b455e622 100644 --- a/server/nativeapi/missing.go +++ b/server/nativeapi/missing.go @@ -63,7 +63,7 @@ func (r *missingRepository) EntityName() string { return "missing_files" } -func deleteMissingFiles(missingFiles core.MissingFiles) http.HandlerFunc { +func deleteMissingFiles(maintenance core.Maintenance) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -72,9 +72,9 @@ func deleteMissingFiles(missingFiles core.MissingFiles) http.HandlerFunc { var err error if len(ids) == 0 { - err = missingFiles.DeleteAllMissingFiles(ctx) + err = maintenance.DeleteAllMissingFiles(ctx) } else { - err = missingFiles.DeleteMissingFiles(ctx, ids) + err = maintenance.DeleteMissingFiles(ctx, ids) } if len(ids) == 1 && errors.Is(err, model.ErrNotFound) { diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 92c4423fd..969650e0a 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -22,16 +22,16 @@ import ( type Router struct { http.Handler - ds model.DataStore - share core.Share - playlists core.Playlists - insights metrics.Insights - libs core.Library - missingFiles core.MissingFiles + ds model.DataStore + share core.Share + playlists core.Playlists + insights metrics.Insights + libs core.Library + maintenance core.Maintenance } -func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library, missingFiles core.MissingFiles) *Router { - r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, missingFiles: missingFiles} +func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library, maintenance core.Maintenance) *Router { + r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, maintenance: maintenance} r.Handler = r.routes() return r } @@ -173,7 +173,7 @@ func (api *Router) addQueueRoute(r chi.Router) { func (api *Router) addMissingFilesRoute(r chi.Router) { r.Route("/missing", func(r chi.Router) { api.RX(r, "/", newMissingRepository(api.ds), false) - r.Delete("/", deleteMissingFiles(api.missingFiles)) + r.Delete("/", deleteMissingFiles(api.maintenance)) }) } From 98a8a68a78cc8a8fb12cb8db0cd1e682cad427bc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 8 Nov 2025 23:20:33 +0000 Subject: [PATCH 5/5] Initial plan