fix: address second code review findings

- Add context cancellation check between pagination pages in GetArtistReleaseGroups
  for responsive graceful shutdown during large discography fetches.
- Eliminate double DB query on cache hit by having GetCachedReleases return
  []ExternalRelease directly instead of just a count, avoiding a redundant
  second query in SyncArtistDiscography.
- Update cache_test.go to match new GetCachedReleases return type.
- Format main.go (pre-existing whitespace issue).

internal/musicbrainz/api.go

@@ -60,6 +60,12 @@ func (c *MusicBrainzClient) GetArtistReleaseGroups(ctx context.Context, artistMB
 		if offset+len(parsed.ReleaseGroups) >= parsed.Count {
 			break
 		}
+
+		// Check context cancellation between pages for responsive shutdown.
+		if err := ctx.Err(); err != nil {
+			return nil, fmt.Errorf("fetch release groups for artist %s: %w", artistMBID, err)
+		}
+
 		offset += limit
 	}
 

internal/musicbrainz/cache.go

@@ -9,11 +9,11 @@ import (
 
 // GetCachedReleases queries the external_releases table for entries
 // belonging to the given artist that were cached within the specified TTL.
-// It returns the count of cached entries and any error encountered.
+// It returns the cached releases and any error encountered.
-func GetCachedReleases(db *database.DB, artistID string, ttl time.Duration) (int, error) {
+func GetCachedReleases(db *database.DB, artistID string, ttl time.Duration) ([]database.ExternalRelease, error) {
 	releases, err := database.GetExternalReleasesByArtistWithCache(db, artistID, ttl)
 	if err != nil {
-		return 0, fmt.Errorf("get cached releases: %w", err)
+		return nil, fmt.Errorf("get cached releases: %w", err)
 	}
-	return len(releases), nil
+	return releases, nil
 }

internal/musicbrainz/cache_test.go

@@ -45,13 +45,13 @@ func TestGetCachedReleases_CacheHit(t *testing.T) {
 	}
 
 	ttl := 24 * time.Hour
-	count, err := GetCachedReleases(db, artistID, ttl)
+	releases, err := GetCachedReleases(db, artistID, ttl)
 	if err != nil {
 		t.Fatalf("GetCachedReleases() error: %v", err)
 	}
 
-	if count != 2 {
+	if len(releases) != 2 {
-		t.Errorf("GetCachedReleases() = %d, want 2", count)
+		t.Errorf("GetCachedReleases() returned %d releases, want 2", len(releases))
 	}
 }
 
@@ -78,13 +78,13 @@ func TestGetCachedReleases_CacheMiss_Expired(t *testing.T) {
 	}
 
 	ttl := 24 * time.Hour
-	count, err := GetCachedReleases(db, artistID, ttl)
+	releases, err := GetCachedReleases(db, artistID, ttl)
 	if err != nil {
 		t.Fatalf("GetCachedReleases() error: %v", err)
 	}
 
-	if count != 0 {
+	if len(releases) != 0 {
-		t.Errorf("GetCachedReleases() = %d, want 0 (expired entry should not be cached)", count)
+		t.Errorf("GetCachedReleases() returned %d releases, want 0 (expired entry should not be cached)", len(releases))
 	}
 }
 
@@ -110,13 +110,13 @@ func TestGetCachedReleases_CacheMiss_NoCachedAt(t *testing.T) {
 	}
 
 	ttl := 24 * time.Hour
-	count, err := GetCachedReleases(db, artistID, ttl)
+	releases, err := GetCachedReleases(db, artistID, ttl)
 	if err != nil {
 		t.Fatalf("GetCachedReleases() error: %v", err)
 	}
 
-	if count != 0 {
+	if len(releases) != 0 {
-		t.Errorf("GetCachedReleases() = %d, want 0 (NULL cached_at should not be cached)", count)
+		t.Errorf("GetCachedReleases() returned %d releases, want 0 (NULL cached_at should not be cached)", len(releases))
 	}
 }
 
@@ -128,13 +128,13 @@ func TestGetCachedReleases_EmptyArtist(t *testing.T) {
 	defer db.Close()
 
 	ttl := 24 * time.Hour
-	count, err := GetCachedReleases(db, "nonexistent-artist", ttl)
+	releases, err := GetCachedReleases(db, "nonexistent-artist", ttl)
 	if err != nil {
 		t.Fatalf("GetCachedReleases() error: %v", err)
 	}
 
-	if count != 0 {
+	if len(releases) != 0 {
-		t.Errorf("GetCachedReleases() = %d, want 0 for nonexistent artist", count)
+		t.Errorf("GetCachedReleases() returned %d releases, want 0 for nonexistent artist", len(releases))
 	}
 }
 
@@ -170,12 +170,15 @@ func TestGetCachedReleases_MixedExpiry(t *testing.T) {
 	}
 
 	ttl := 24 * time.Hour
-	count, err := GetCachedReleases(db, artistID, ttl)
+	releases, err := GetCachedReleases(db, artistID, ttl)
 	if err != nil {
 		t.Fatalf("GetCachedReleases() error: %v", err)
 	}
 
-	if count != 1 {
+	if len(releases) != 1 {
-		t.Errorf("GetCachedReleases() = %d, want 1 (only fresh entry)", count)
+		t.Errorf("GetCachedReleases() returned %d releases, want 1 (only fresh entry)", len(releases))
+	}
+	if len(releases) > 0 && releases[0].RGID != "rg-fresh" {
+		t.Errorf("expected rg-fresh, got %s", releases[0].RGID)
 	}
 }

internal/musicbrainz/sync.go

@@ -32,17 +32,17 @@ func SyncArtistDiscography(
 	}
 
 	// Step 1: Check cache.
-	cachedCount, err := GetCachedReleases(db, artistMBID, ttl)
+	cachedReleases, err := GetCachedReleases(db, artistMBID, ttl)
 	if err != nil {
 		return nil, fmt.Errorf("sync artist discography: cache check failed: %w", err)
 	}
 
 	// Step 2: If we have cached data, return it.
-	if cachedCount > 0 {
+	if len(cachedReleases) > 0 {
 		if err := ctx.Err(); err != nil {
 			return nil, fmt.Errorf("sync artist discography: %w", err)
 		}
-		return database.GetExternalReleasesByArtistWithCache(db, artistMBID, ttl)
+		return cachedReleases, nil
 	}
 
 	// Step 3: Cache miss — fetch from MusicBrainz API.