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