From 319651d7d1a0cd3004df04906190f0c762d60f01 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 8 Nov 2025 18:03:09 -0500 Subject: [PATCH] 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 +}