From 73519898ebd9702c43caf480c23ae853542adc68 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 25 Jul 2026 14:22:45 -0400 Subject: [PATCH] fix(artwork): make the pool-split worker test race-clean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI runs the suite under -race, which the local `make test` does not, so this only showed up there: 230 specs passed and the detector still failed the run. The drain-pools spec started Run and never waited for it, so pool goroutines outlived the spec and raced the config snapshot Ginkgo restores on cleanup. It now cancels, unparks the blocked lookups and waits for Run to return. Two test doubles also had to become concurrency-safe, since the spec is the first to resolve several artists at once: fakeImageAgent's call counters, and MockAlbumRepo.GetAll, which records the last query options on a read path. MockDataStore's lazy accessors get the same treatment — only MediaFile was guarded before, and two pools now reach them concurrently. ArtworkQueue takes an unlocked helper for its internal Artwork call, since repoMu is not reentrant. --- core/artwork/agent_images_test.go | 5 +++ core/artwork/worker_test.go | 12 +++++-- tests/mock_album_repo.go | 5 +++ tests/mock_data_store.go | 52 +++++++++++++++++++++++++++++-- 4 files changed, 68 insertions(+), 6 deletions(-) diff --git a/core/artwork/agent_images_test.go b/core/artwork/agent_images_test.go index d8332d66a..8525ab377 100644 --- a/core/artwork/agent_images_test.go +++ b/core/artwork/agent_images_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" @@ -30,6 +31,8 @@ type fakeImageAgent struct { gotAlbumName string // block, when set, holds every lookup until closed, standing in for a slow/rate-limited agent. block chan struct{} + // mu guards the call counters: the worker resolves several items concurrently. + mu sync.Mutex } func (f *fakeImageAgent) AgentName() string { return f.name } @@ -38,8 +41,10 @@ func (f *fakeImageAgent) GetArtistImages(_ context.Context, _, name, _ string) ( if f.block != nil { <-f.block } + f.mu.Lock() f.artistCalls++ f.gotArtistName = name + f.mu.Unlock() return f.imgs, f.err } diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index 794f9190f..076b1cbc9 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -631,7 +631,6 @@ var _ = Describe("Worker", func() { // Every artist lookup blocks until released, standing in for the rate limiter. block := make(chan struct{}) - DeferCleanup(func() { close(block) }) artists := model.Artists{} for i := range 8 { artists = append(artists, model.Artist{ID: fmt.Sprintf("arx%d", i), Name: "A"}) @@ -650,8 +649,15 @@ var _ = Describe("Worker", func() { })).To(Succeed()) runCtx, cancel := context.WithCancel(ctx) - DeferCleanup(cancel) - go func() { _ = w.Run(runCtx) }() + done := make(chan struct{}) + go func() { defer close(done); _ = w.Run(runCtx) }() + // Wait for Run to return: a leaked pool goroutine outlives the spec and races the + // config snapshot Ginkgo restores on cleanup. + DeferCleanup(func() { + cancel() + close(block) // unpark the blocked lookups so the pools can unwind + <-done + }) // The album must land while every artist is still parked in the agent. Eventually(func() bool { diff --git a/tests/mock_album_repo.go b/tests/mock_album_repo.go index 17539255b..aa72d9925 100644 --- a/tests/mock_album_repo.go +++ b/tests/mock_album_repo.go @@ -2,6 +2,7 @@ package tests import ( "errors" + "sync" "time" "github.com/navidrome/navidrome/model" @@ -20,6 +21,7 @@ type MockAlbumRepo struct { All model.Albums Err bool Options model.QueryOptions + optionsMu sync.Mutex SearchQuery string // last query passed to Search ReassignAnnotationCalls map[string]string // prevID -> newID CopyAttributesCalls map[string]string // fromID -> toID @@ -68,7 +70,10 @@ func (m *MockAlbumRepo) Put(al *model.Album) error { func (m *MockAlbumRepo) GetAll(qo ...model.QueryOptions) (model.Albums, error) { if len(qo) > 0 { + // Recording the last options is a read-path write, and callers resolve concurrently. + m.optionsMu.Lock() m.Options = qo[0] + m.optionsMu.Unlock() } if m.Err { return nil, errors.New("unexpected error") diff --git a/tests/mock_data_store.go b/tests/mock_data_store.go index d8585fabf..32f56a4f0 100644 --- a/tests/mock_data_store.go +++ b/tests/mock_data_store.go @@ -39,6 +39,8 @@ type MockDataStore struct { } func (db *MockDataStore) Library(ctx context.Context) model.LibraryRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedLibrary != nil { return db.MockedLibrary } @@ -50,6 +52,8 @@ func (db *MockDataStore) Library(ctx context.Context) model.LibraryRepository { } func (db *MockDataStore) Folder(ctx context.Context) model.FolderRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedFolder != nil { return db.MockedFolder } @@ -61,6 +65,8 @@ func (db *MockDataStore) Folder(ctx context.Context) model.FolderRepository { } func (db *MockDataStore) Tag(ctx context.Context) model.TagRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedTag != nil { return db.MockedTag } @@ -72,6 +78,8 @@ func (db *MockDataStore) Tag(ctx context.Context) model.TagRepository { } func (db *MockDataStore) Album(ctx context.Context) model.AlbumRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedAlbum != nil { return db.MockedAlbum } @@ -83,6 +91,8 @@ func (db *MockDataStore) Album(ctx context.Context) model.AlbumRepository { } func (db *MockDataStore) Artist(ctx context.Context) model.ArtistRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedArtist != nil { return db.MockedArtist } @@ -94,11 +104,11 @@ func (db *MockDataStore) Artist(ctx context.Context) model.ArtistRepository { } func (db *MockDataStore) MediaFile(ctx context.Context) model.MediaFileRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.RealDS != nil && db.MockedMediaFile == nil { return db.RealDS.MediaFile(ctx) } - db.repoMu.Lock() - defer db.repoMu.Unlock() if db.MockedMediaFile == nil { db.MockedMediaFile = CreateMockMediaFileRepo() } @@ -106,6 +116,8 @@ func (db *MockDataStore) MediaFile(ctx context.Context) model.MediaFileRepositor } func (db *MockDataStore) Genre(ctx context.Context) model.GenreRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedGenre != nil { return db.MockedGenre } @@ -117,6 +129,8 @@ func (db *MockDataStore) Genre(ctx context.Context) model.GenreRepository { } func (db *MockDataStore) Playlist(ctx context.Context) model.PlaylistRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedPlaylist != nil { return db.MockedPlaylist } @@ -128,6 +142,8 @@ func (db *MockDataStore) Playlist(ctx context.Context) model.PlaylistRepository } func (db *MockDataStore) PlayQueue(ctx context.Context) model.PlayQueueRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedPlayQueue != nil { return db.MockedPlayQueue } @@ -139,6 +155,8 @@ func (db *MockDataStore) PlayQueue(ctx context.Context) model.PlayQueueRepositor } func (db *MockDataStore) UserProps(ctx context.Context) model.UserPropsRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedUserProps != nil { return db.MockedUserProps } @@ -150,6 +168,8 @@ func (db *MockDataStore) UserProps(ctx context.Context) model.UserPropsRepositor } func (db *MockDataStore) Property(ctx context.Context) model.PropertyRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedProperty != nil { return db.MockedProperty } @@ -161,6 +181,8 @@ func (db *MockDataStore) Property(ctx context.Context) model.PropertyRepository } func (db *MockDataStore) Share(ctx context.Context) model.ShareRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedShare != nil { return db.MockedShare } @@ -172,6 +194,8 @@ func (db *MockDataStore) Share(ctx context.Context) model.ShareRepository { } func (db *MockDataStore) User(ctx context.Context) model.UserRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedUser != nil { return db.MockedUser } @@ -183,6 +207,8 @@ func (db *MockDataStore) User(ctx context.Context) model.UserRepository { } func (db *MockDataStore) Transcoding(ctx context.Context) model.TranscodingRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedTranscoding != nil { return db.MockedTranscoding } @@ -194,6 +220,8 @@ func (db *MockDataStore) Transcoding(ctx context.Context) model.TranscodingRepos } func (db *MockDataStore) Player(ctx context.Context) model.PlayerRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedPlayer != nil { return db.MockedPlayer } @@ -205,6 +233,8 @@ func (db *MockDataStore) Player(ctx context.Context) model.PlayerRepository { } func (db *MockDataStore) ScrobbleBuffer(ctx context.Context) model.ScrobbleBufferRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.RealDS != nil && db.MockedScrobbleBuffer == nil { return db.RealDS.ScrobbleBuffer(ctx) } @@ -217,6 +247,8 @@ func (db *MockDataStore) ScrobbleBuffer(ctx context.Context) model.ScrobbleBuffe } func (db *MockDataStore) Scrobble(ctx context.Context) model.ScrobbleRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedScrobble != nil { return db.MockedScrobble } @@ -228,6 +260,8 @@ func (db *MockDataStore) Scrobble(ctx context.Context) model.ScrobbleRepository } func (db *MockDataStore) Radio(ctx context.Context) model.RadioRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedRadio != nil { return db.MockedRadio } @@ -239,6 +273,8 @@ func (db *MockDataStore) Radio(ctx context.Context) model.RadioRepository { } func (db *MockDataStore) Plugin(ctx context.Context) model.PluginRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedPlugin != nil { return db.MockedPlugin } @@ -250,6 +286,14 @@ func (db *MockDataStore) Plugin(ctx context.Context) model.PluginRepository { } func (db *MockDataStore) Artwork(ctx context.Context) model.ArtworkRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() + return db.artworkLocked(ctx) +} + +// artworkLocked is the body of Artwork for callers already holding repoMu; repoMu is a plain +// Mutex, so re-entering through the exported method would deadlock. +func (db *MockDataStore) artworkLocked(ctx context.Context) model.ArtworkRepository { if db.MockedArtwork != nil { return db.MockedArtwork } @@ -261,6 +305,8 @@ func (db *MockDataStore) Artwork(ctx context.Context) model.ArtworkRepository { } func (db *MockDataStore) ArtworkQueue(ctx context.Context) model.ArtworkQueueRepository { + db.repoMu.Lock() + defer db.repoMu.Unlock() if db.MockedArtworkQueue != nil { return db.MockedArtworkQueue } @@ -268,7 +314,7 @@ func (db *MockDataStore) ArtworkQueue(ctx context.Context) model.ArtworkQueueRep return db.RealDS.ArtworkQueue(ctx) } q := CreateMockArtworkQueueRepo() - if aw, ok := db.Artwork(ctx).(*MockArtworkRepo); ok { + if aw, ok := db.artworkLocked(ctx).(*MockArtworkRepo); ok { q.ItemArtworkSource = aw } db.MockedArtworkQueue = q