From e5e2d860ef2c7440a4b597bfe02ea67bea175d7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Mon, 23 Jun 2025 13:26:48 -0400 Subject: [PATCH] fix(scanner): ensure full scans update the DB (#4252) * fix: ensure full scan refreshes all artist stats After PR #4059, full scans were not forcing a refresh of all artists. This change ensures that during full scans, all artist stats are refreshed instead of only those with recently updated media files. Changes: - Set changesDetected=true at start of full scans to ensure maintenance operations run - Add allArtists parameter to RefreshStats() method - Pass fullScan state to RefreshStats to control refresh scope - Update mock repository to match new interface Fixes #4246 Related to PR #4059 * fix: add tests for full and incremental scans Signed-off-by: Deluan --------- Signed-off-by: Deluan --- model/artist.go | 2 +- persistence/artist_repository.go | 25 ++++++++----- scanner/scanner.go | 17 +++++++-- scanner/scanner_test.go | 60 ++++++++++++++++++++++++++++++++ tests/mock_artist_repo.go | 14 ++++++++ 5 files changed, 106 insertions(+), 12 deletions(-) diff --git a/model/artist.go b/model/artist.go index 68836ff28..7f68f9787 100644 --- a/model/artist.go +++ b/model/artist.go @@ -82,7 +82,7 @@ type ArtistRepository interface { // The following methods are used exclusively by the scanner: RefreshPlayCounts() (int64, error) - RefreshStats() (int64, error) + RefreshStats(allArtists bool) (int64, error) AnnotatedRepository SearchableRepository[Artists] diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index c656950ce..b6ce42128 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -292,25 +292,34 @@ on conflict (user_id, item_id, item_type) do update } // RefreshStats updates the stats field for artists whose associated media files were updated after the oldest recorded library scan time. -// It processes artists in batches to handle potentially large updates. -func (r *artistRepository) RefreshStats() (int64, error) { - touchedArtistsQuerySQL := ` +// When allArtists is true, it refreshes stats for all artists. It processes artists in batches to handle potentially large updates. +func (r *artistRepository) RefreshStats(allArtists bool) (int64, error) { + var allTouchedArtistIDs []string + if allArtists { + // Refresh stats for all artists + allArtistsQuerySQL := `SELECT DISTINCT id FROM artist WHERE id <> ''` + if err := r.db.NewQuery(allArtistsQuerySQL).Column(&allTouchedArtistIDs); err != nil { + return 0, fmt.Errorf("fetching all artist IDs: %w", err) + } + log.Debug(r.ctx, "RefreshStats: Refreshing all artists.", "count", len(allTouchedArtistIDs)) + } else { + // Only refresh artists with updated media files + touchedArtistsQuerySQL := ` SELECT DISTINCT mfa.artist_id FROM media_file_artists mfa JOIN media_file mf ON mfa.media_file_id = mf.id WHERE mf.updated_at > (SELECT last_scan_at FROM library ORDER BY last_scan_at ASC LIMIT 1) ` - - var allTouchedArtistIDs []string - if err := r.db.NewQuery(touchedArtistsQuerySQL).Column(&allTouchedArtistIDs); err != nil { - return 0, fmt.Errorf("fetching touched artist IDs: %w", err) + if err := r.db.NewQuery(touchedArtistsQuerySQL).Column(&allTouchedArtistIDs); err != nil { + return 0, fmt.Errorf("fetching touched artist IDs: %w", err) + } + log.Debug(r.ctx, "RefreshStats: Refreshing touched artists.", "count", len(allTouchedArtistIDs)) } if len(allTouchedArtistIDs) == 0 { log.Debug(r.ctx, "RefreshStats: No artists to update.") return 0, nil } - log.Debug(r.ctx, "RefreshStats: Found artists to update.", "count", len(allTouchedArtistIDs)) // Template for the batch update with placeholder markers that we'll replace batchUpdateStatsSQL := ` diff --git a/scanner/scanner.go b/scanner/scanner.go index d84c58a3e..7ddc78b17 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -45,14 +45,25 @@ func (s *scanState) sendError(err error) { } func (s *scannerImpl) scanAll(ctx context.Context, fullScan bool, progress chan<- *ProgressInfo) { - state := scanState{progress: progress, fullScan: fullScan} + startTime := time.Now() + + state := scanState{ + progress: progress, + fullScan: fullScan, + changesDetected: atomic.Bool{}, + } + + // Set changesDetected to true for full scans to ensure all maintenance operations run + if fullScan { + state.changesDetected.Store(true) + } + libs, err := s.ds.Library(ctx).GetAll() if err != nil { state.sendWarning(fmt.Sprintf("getting libraries: %s", err)) return } - startTime := time.Now() log.Info(ctx, "Scanner: Starting scan", "fullScan", state.fullScan, "numLibraries", len(libs)) // Store scan type and start time @@ -148,7 +159,7 @@ func (s *scannerImpl) runRefreshStats(ctx context.Context, state *scanState) fun return nil } start := time.Now() - stats, err := s.ds.Artist(ctx).RefreshStats() + stats, err := s.ds.Artist(ctx).RefreshStats(state.fullScan) if err != nil { log.Error(ctx, "Scanner: Error refreshing artists stats", err) return fmt.Errorf("refreshing artists stats: %w", err) diff --git a/scanner/scanner_test.go b/scanner/scanner_test.go index 3ed5a4704..106c6e9c2 100644 --- a/scanner/scanner_test.go +++ b/scanner/scanner_test.go @@ -612,6 +612,56 @@ var _ = Describe("Scanner", Ordered, func() { }) }) }) + + Describe("RefreshStats", func() { + var refreshStatsCalls []bool + + BeforeEach(func() { + refreshStatsCalls = nil + + // Create a mock artist repository that tracks RefreshStats calls + originalArtistRepo := ds.RealDS.Artist(ctx) + ds.MockedArtist = &testArtistRepo{ + ArtistRepository: originalArtistRepo, + callTracker: &refreshStatsCalls, + } + + // Create a simple filesystem for testing + revolver := template(_t{"albumartist": "The Beatles", "album": "Revolver", "year": 1966}) + createFS(fstest.MapFS{ + "The Beatles/Revolver/01 - Taxman.mp3": revolver(track(1, "Taxman")), + }) + }) + + It("should call RefreshStats with allArtists=true for full scans", func() { + Expect(runScanner(ctx, true)).To(Succeed()) + + Expect(refreshStatsCalls).To(HaveLen(1)) + Expect(refreshStatsCalls[0]).To(BeTrue(), "RefreshStats should be called with allArtists=true for full scans") + }) + + It("should call RefreshStats with allArtists=false for incremental scans", func() { + // First do a full scan to set up the data + Expect(runScanner(ctx, true)).To(Succeed()) + + // Reset the tracker to only track the incremental scan + refreshStatsCalls = nil + + // Add a new file to trigger changes detection + revolver := template(_t{"albumartist": "The Beatles", "album": "Revolver", "year": 1966}) + fsys := createFS(fstest.MapFS{ + "The Beatles/Revolver/01 - Taxman.mp3": revolver(track(1, "Taxman")), + "The Beatles/Revolver/02 - Eleanor Rigby.mp3": revolver(track(2, "Eleanor Rigby")), + }) + _ = fsys + + // Do an incremental scan + Expect(runScanner(ctx, false)).To(Succeed()) + + Expect(refreshStatsCalls).To(HaveLen(1)) + Expect(refreshStatsCalls[0]).To(BeFalse(), "RefreshStats should be called with allArtists=false for incremental scans") + }) + }) }) func createFindByPath(ctx context.Context, ds model.DataStore) func(string) (*model.MediaFile, error) { @@ -638,3 +688,13 @@ func (m *mockMediaFileRepo) GetMissingAndMatching(libId int) (model.MediaFileCur } return m.MediaFileRepository.GetMissingAndMatching(libId) } + +type testArtistRepo struct { + model.ArtistRepository + callTracker *[]bool +} + +func (m *testArtistRepo) RefreshStats(allArtists bool) (int64, error) { + *m.callTracker = append(*m.callTracker, allArtists) + return m.ArtistRepository.RefreshStats(allArtists) +} diff --git a/tests/mock_artist_repo.go b/tests/mock_artist_repo.go index 7058cead0..da5851061 100644 --- a/tests/mock_artist_repo.go +++ b/tests/mock_artist_repo.go @@ -94,4 +94,18 @@ func (m *MockArtistRepo) UpdateExternalInfo(artist *model.Artist) error { return nil } +func (m *MockArtistRepo) RefreshStats(allArtists bool) (int64, error) { + if m.Err { + return 0, errors.New("mock repo error") + } + return int64(len(m.Data)), nil +} + +func (m *MockArtistRepo) RefreshPlayCounts() (int64, error) { + if m.Err { + return 0, errors.New("mock repo error") + } + return int64(len(m.Data)), nil +} + var _ model.ArtistRepository = (*MockArtistRepo)(nil)