From 2baf586607c5193b29640645fe109e66e8a774d3 Mon Sep 17 00:00:00 2001
From: Vladimir Zagainov <mrixs@mrixs.ru>
Date: Tue, 26 May 2026 14:55:50 +0300
Subject: [PATCH] fix: address second code review findings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Add CacheTTL default (24h) in config applyDefaults — without this,
  omitting cache_ttl from config silently defeats the entire caching
  mechanism (TTL=0 means cached data is never served)
- Fix pagination to use total Count instead of checking if last page
  was short — avoids wasting a rate-limit token when total count is
  an exact multiple of 100
- Preserve user-set IsIgnored flags across re-syncs — previously,
  DELETE+INSERT in the sync transaction reset all ignore flags to
  false, losing user preferences on every cache-expiry re-sync
- Check context cancellation on cache-hit code path — previously,
  ctx.Err() was not checked between cache check and returning cached
  data, violating the cancellation contract
---
 internal/config/config.go        |  3 +++
 internal/musicbrainz/api.go      |  4 ++--
 internal/musicbrainz/api_test.go |  4 ++--
 internal/musicbrainz/sync.go     | 24 ++++++++++++++++++++++++
 4 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/internal/config/config.go b/internal/config/config.go
index 0624d0e..107ac93 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -86,6 +86,9 @@ func applyDefaults(cfg *Config) {
 	if cfg.Scanner.FuzzyThreshold == 0 {
 		cfg.Scanner.FuzzyThreshold = 0.85
 	}
+	if cfg.MusicBrainz.CacheTTL == 0 {
+		cfg.MusicBrainz.CacheTTL = 24 * time.Hour
+	}
 }
 
 // validate checks that required fields are set and values are within acceptable ranges.
diff --git a/internal/musicbrainz/api.go b/internal/musicbrainz/api.go
index 334b6a5..0658ef2 100644
--- a/internal/musicbrainz/api.go
+++ b/internal/musicbrainz/api.go
@@ -56,8 +56,8 @@ func (c *MusicBrainzClient) GetArtistReleaseGroups(ctx context.Context, artistMB
 
 		allGroups = append(allGroups, parsed.ReleaseGroups...)
 
-		// If we got fewer results than the limit, we've reached the end
-		if len(parsed.ReleaseGroups) < limit {
+		// If we've fetched all results, we've reached the end.
+		if offset+len(parsed.ReleaseGroups) >= parsed.Count {
 			break
 		}
 		offset += limit
diff --git a/internal/musicbrainz/api_test.go b/internal/musicbrainz/api_test.go
index 26bea62..c7d1980 100644
--- a/internal/musicbrainz/api_test.go
+++ b/internal/musicbrainz/api_test.go
@@ -242,7 +242,7 @@ func TestGetArtistReleaseGroups_Pagination(t *testing.T) {
 			// First page: return 100 results (full page, matching limit) to trigger pagination
 			xml := `<?xml version="1.0" encoding="UTF-8"?>
 <metadata xmlns="http://musicbrainz.org/ns/mmd-2.0#">
-  <release-group-list count="150">`
+  <release-group-list count="101">`
 			for i := 0; i < 100; i++ {
 				xml += `
     <release-group id="rg-page1-` + string(rune('0'+i%10)) + `" type="Album">
@@ -263,7 +263,7 @@ func TestGetArtistReleaseGroups_Pagination(t *testing.T) {
 			// Second page: return only 1 result (< limit, signaling last page)
 			w.Write([]byte(`<?xml version="1.0" encoding="UTF-8"?>
 <metadata xmlns="http://musicbrainz.org/ns/mmd-2.0#">
-  <release-group-list count="150">
+  <release-group-list count="101">
     <release-group id="rg-page2" type="Album">
       <title>Page 2 Album</title>
       <first-release-date>2021-01-01</first-release-date>
diff --git a/internal/musicbrainz/sync.go b/internal/musicbrainz/sync.go
index 606e899..a65921c 100644
--- a/internal/musicbrainz/sync.go
+++ b/internal/musicbrainz/sync.go
@@ -39,6 +39,9 @@ func SyncArtistDiscography(
 
 	// Step 2: If we have cached data, return it.
 	if cachedCount > 0 {
+		if err := ctx.Err(); err != nil {
+			return nil, fmt.Errorf("sync artist discography: %w", err)
+		}
 		return database.GetExternalReleasesByArtistWithCache(db, artistMBID, ttl)
 	}
 
@@ -59,6 +62,23 @@ func SyncArtistDiscography(
 	}
 	defer tx.Rollback()
 
+	// Read existing ignore states before deleting to preserve user-set flags.
+	ignoredMap := map[string]bool{}
+	rows, err := tx.Query("SELECT rgid, is_ignored FROM external_releases WHERE artist_id = ?", artistMBID)
+	if err != nil {
+		return nil, fmt.Errorf("sync artist discography: query existing releases: %w", err)
+	}
+	for rows.Next() {
+		var rgid string
+		var ignored bool
+		if err := rows.Scan(&rgid, &ignored); err != nil {
+			rows.Close()
+			return nil, fmt.Errorf("sync artist discography: scan existing release: %w", err)
+		}
+		ignoredMap[rgid] = ignored
+	}
+	rows.Close()
+
 	// Delete old entries for this artist to avoid stale records.
 	if _, err := tx.Exec("DELETE FROM external_releases WHERE artist_id = ?", artistMBID); err != nil {
 		return nil, fmt.Errorf("sync artist discography: delete old releases: %w", err)
@@ -73,6 +93,10 @@ func SyncArtistDiscography(
 
 		ext := rg.ToExternalRelease()
 		ext.CachedAt = now
+		// Preserve user-set ignore flag from previous sync.
+		if ignored, ok := ignoredMap[ext.RGID]; ok {
+			ext.IsIgnored = ignored
+		}
 
 		cachedAtStr := ext.CachedAt.Format("2006-01-02 15:04:05")
 		if _, err := tx.Exec(