fix: address code review findings

cmd/naviwatcher/main.go

@@ -84,6 +84,9 @@ func NewApp(ctx context.Context, cfg *config.Config) (*App, error) {
 
 // Close cleans up all application resources in reverse order of initialization.
 func (a *App) Close() {
+	if a.mbClient != nil {
+		a.mbClient.Close()
+	}
 	if a.db != nil {
 		if err := a.db.Close(); err != nil {
 			log.Printf("Error closing database: %v", err)

internal/database/database.go

@@ -15,7 +15,11 @@ type DB struct {
 
 // New opens a SQLite database at dbPath and runs schema migrations.
 func New(dbPath string) (*DB, error) {
-	conn, err := sql.Open("sqlite3", dbPath)
+	// The _foreign_keys=on DSN parameter enables foreign key enforcement on
+	// EVERY connection in the pool. A one-off "PRAGMA foreign_keys=ON" executed
+	// on the pooled *sql.DB only applies to the first connection and is lost on
+	// connections opened later by the pool, silently disabling the safety net.
+	conn, err := sql.Open("sqlite3", dbPath+"?_foreign_keys=on")
 	if err != nil {
 		return nil, fmt.Errorf("open database: %w", err)
 	}
@@ -26,12 +30,6 @@ func New(dbPath string) (*DB, error) {
 		return nil, fmt.Errorf("set WAL mode: %w", err)
 	}
 
-	// Enable foreign key enforcement.
-	if _, err := conn.Exec("PRAGMA foreign_keys=ON"); err != nil {
-		conn.Close()
-		return nil, fmt.Errorf("enable foreign keys: %w", err)
-	}
-
 	// Set busy timeout to handle concurrent write contention.
 	if _, err := conn.Exec("PRAGMA busy_timeout=5000"); err != nil {
 		conn.Close()

internal/database/external_releases.go

@@ -119,28 +119,6 @@ func SetReleaseIgnored(db *DB, rgid string, ignored bool) error {
 	return nil
 }
 
-// DeleteExternalReleasesByArtist removes all external_release rows for a given artist_id.
-func DeleteExternalReleasesByArtist(db *DB, artistID string) error {
-	_, err := db.Conn().Exec(
-		"DELETE FROM external_releases WHERE artist_id = ?",
-		artistID,
-	)
-	if err != nil {
-		return fmt.Errorf("delete external releases by artist: %w", err)
-	}
-	return nil
-}
-
-// CountExternalReleases returns the total number of external_release rows.
-func CountExternalReleases(db *DB) (int, error) {
-	var count int
-	err := db.Conn().QueryRow("SELECT COUNT(*) FROM external_releases").Scan(&count)
-	if err != nil {
-		return 0, fmt.Errorf("count external releases: %w", err)
-	}
-	return count, nil
-}
-
 // GetExternalReleasesByArtistWithCache returns cached external_release rows for a given artist_id
 // that are within the specified TTL.
 func GetExternalReleasesByArtistWithCache(db *DB, artistID string, ttl time.Duration) ([]ExternalRelease, error) {

internal/musicbrainz/api.go

@@ -6,7 +6,6 @@ import (
 	"net/url"
 
 	"naviwatcher/internal/database"
-	"naviwatcher/internal/normalize"
 )
 
 // excludedStatuses contains release-group statuses that should be filtered out.
@@ -112,20 +111,6 @@ func IsTypeIncluded(releaseType string) bool {
 	return includedTypes[releaseType]
 }
 
-// NormalizeString normalizes a string for fuzzy matching.
-// It delegates to the shared normalize package; see normalize.NormalizeString
-// for the full normalization contract.
-func NormalizeString(s string) string {
-	return normalize.NormalizeString(s)
-}
-
-// NormalizeArtistName normalizes an artist name for comparison.
-// It delegates to the shared normalize package; see
-// normalize.NormalizeArtistName for the full normalization contract.
-func NormalizeArtistName(name string) string {
-	return normalize.NormalizeArtistName(name)
-}
-
 // ToExternalRelease converts a ReleaseGroup to an ExternalRelease for database
 // persistence. artistID is the canonical artist key from artist_settings (the
 // Navidrome artist ID), which is what external_releases.artist_id references and

internal/musicbrainz/api_test.go

@@ -93,81 +93,6 @@ func TestFilterReleaseGroups_IgnoreCompilations(t *testing.T) {
 	}
 }
 
-// ---------- NormalizeString tests ----------
-
-func TestNormalizeString_Basic(t *testing.T) {
-	tests := []struct {
-		input    string
-		expected string
-	}{
-		// Lowercase conversion
-		{"DARK SIDE OF THE MOON", "dark side of the moon"},
-		// Special character removal
-		{"Dark Side of the Moon!", "dark side of the moon"},
-		{"Dark-Side-of-the-Moon", "dark side of the moon"},
-		{"Dark_Side_of_the_Moon", "dark side of the moon"},
-		// Bracket removal
-		{"Dark Side of the Moon [Deluxe Edition]", "dark side of the moon"},
-		{"Dark Side of the Moon [Remastered 2020]", "dark side of the moon"},
-		{"Album [2023 Remix]", "album"},
-		// Parenthesis removal
-		{"Dark Side of the Moon (Deluxe)", "dark side of the moon"},
-		{"Album (Remastered)", "album"},
-		// Year removal
-		{"Dark Side of the Moon 1973", "dark side of the moon"},
-		{"Album 2020 Remastered", "album remastered"},
-		// Space collapsing
-		{"Dark   Side   of   the   Moon", "dark side of the moon"},
-		// Trim
-		{"  Dark Side of the Moon  ", "dark side of the moon"},
-		// Combined
-		{"The Dark Side of the Moon [2011 Remaster] (Deluxe Edition)", "the dark side of the moon"},
-		// Empty
-		{"", ""},
-		// Only special chars
-		{"!@#$%^&*()", ""},
-		// Digits that are not years should stay
-		{"30 Seconds to Mars", "30 seconds to mars"},
-		{"1941 - The Greatest Hits", "the greatest hits"},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.input, func(t *testing.T) {
-			got := NormalizeString(tt.input)
-			if got != tt.expected {
-				t.Errorf("NormalizeString(%q) = %q, want %q", tt.input, got, tt.expected)
-			}
-		})
-	}
-}
-
-func TestNormalizeArtistName(t *testing.T) {
-	tests := []struct {
-		input    string
-		expected string
-	}{
-		{"Pink Floyd", "pink floyd"},
-		{"The Beatles", "beatles"},
-		{"A Perfect Circle", "perfect circle"},
-		{"An Orchestra", "orchestra"},
-		{"  The  Who  ", "who"},
-		{"THE WHO", "who"},
-		// No stripping needed
-		{"Radiohead", "radiohead"},
-		// Already stripped
-		{"Beatles", "beatles"},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.input, func(t *testing.T) {
-			got := NormalizeArtistName(tt.input)
-			if got != tt.expected {
-				t.Errorf("NormalizeArtistName(%q) = %q, want %q", tt.input, got, tt.expected)
-			}
-		})
-	}
-}
-
 // ---------- ReleaseGroup.ToExternalRelease tests ----------
 
 func TestReleaseGroup_ToExternalRelease(t *testing.T) {

internal/musicbrainz/client.go

@@ -35,6 +35,12 @@ func NewClient(cfg config.MusicBrainzConfig) *MusicBrainzClient {
 	}
 }
 
+// Close releases resources held by the client, draining any idle keep-alive
+// connections so they don't linger until garbage collection.
+func (c *MusicBrainzClient) Close() {
+	c.httpClient.CloseIdleConnections()
+}
+
 // doGet performs a rate-limited HTTP GET request to the MusicBrainz API.
 // It blocks until the rate limiter allows the request, then sets the proper
 // User-Agent header and returns the response body.

internal/normalize/normalize.go

@@ -18,6 +18,10 @@ var (
 	parenRe   = regexp.MustCompile(`\([^)]*\)`)
 	yearRe    = regexp.MustCompile(`\b(1[0-9]{3}|2[0-9]{3})\b`)
 	spaceRe   = regexp.MustCompile(`\s+`)
+	// wordRe matches any alphabetic character. Used to decide whether a title
+	// that collapses entirely to a year actually had other words worth keeping
+	// (e.g. "1989 (Deluxe)") versus being a bare year title (e.g. "1989").
+	wordRe = regexp.MustCompile(`[a-z]`)
 )
 
 // NormalizeString normalizes a string for fuzzy matching by:
@@ -28,6 +32,10 @@ var (
 //   - Collapsing multiple spaces into one
 //   - Trimming leading/trailing whitespace
 func NormalizeString(s string) string {
+	// Capture the original input; used after stripping to tell a bare year
+	// title apart from a title that merely collapses to a year.
+	original := s
+
 	// Convert to lowercase
 	s = strings.ToLower(s)
 
@@ -37,11 +45,23 @@ func NormalizeString(s string) string {
 	// Remove parenthesized content (e.g., (Deluxe), (Remastered))
 	s = parenRe.ReplaceAllString(s, "")
 
-	// Remove years (4-digit numbers between 1000-2999). If stripping the
+	// Remove years (4-digit numbers between 1000-2999). If stripping the year
-	// year would empty the entire string (e.g. an album literally titled
+	// would empty the entire string, we must decide what to keep:
-	// "1989" or "2112"), keep the original form so the title can still match.
+	//   - A bare year title (e.g. "1989", "2112") has no other words, so keep
+	//     the year so it can still match itself (the user owns that album).
+	//   - A title that had OTHER words alongside the year (e.g. "1989 (Deluxe)")
+	//     collapses to empty on purpose: it is a distinct release group that
+	//     must NOT be considered already-present just because the user owns the
+	//     standard "1989". Collapsing to empty makes it score 0.0 against a
+	//     plain "1989", correctly reporting the reissue as missing.
 	stripped := yearRe.ReplaceAllString(s, "")
-	if strings.TrimSpace(stripped) != "" {
+	if strings.TrimSpace(stripped) == "" {
+		if wordRe.MatchString(strings.ToLower(original)) {
+			s = ""
+		} else {
+			s = strings.TrimSpace(s)
+		}
+	} else {
 		s = stripped
 	}
 

internal/normalize/normalize_test.go

@@ -39,6 +39,11 @@ func TestNormalizeString_Basic(t *testing.T) {
 		// Year-only title is preserved (not collapsed to empty) so it can still match
 		{"1989", "1989"},
 		{"2112", "2112"},
+		// A year-plus-suffix title collapses to empty: it is a distinct release
+		// group (e.g. "1989 (Deluxe)") and must NOT match a bare "1989".
+		{"1989 (Deluxe)", ""},
+		{"1989 [Deluxe Edition]", ""},
+		{"2112 (Remastered)", ""},
 	}
 
 	for _, tt := range tests {

internal/scanner/scan.go

@@ -2,6 +2,7 @@ package scanner
 
 import (
 	"context"
+	"log"
 
 	"naviwatcher/internal/database"
 )
@@ -55,9 +56,12 @@ func ScanAll(ctx context.Context, db *database.DB, threshold float64) ([]Missing
 		if !s.Monitored {
 			continue
 		}
+		// A transient error for one artist must not abort the whole scan and
+		// take down the daemon; log it and continue with the remaining artists.
 		missing, err := ScanArtist(ctx, db, s.ID, resolved)
 		if err != nil {
-			return all, err
+			log.Printf("scan artist %s failed: %v", s.ID, err)
+			continue
 		}
 		all = append(all, missing...)
 	}

internal/scanner/scan_test.go

@@ -139,6 +139,35 @@ func TestScanArtist_RemasteredVariantNotMissing(t *testing.T) {
 	}
 }
 
+func TestScanArtist_YearTitledAlbumReissueReportedMissing(t *testing.T) {
+	db := newTestDB(t)
+	defer db.Close()
+
+	// Rush "2112" is a bare year-titled album; "2112 (Remastered)" is a
+	// distinct release group. Owning the standard 2112 must NOT count as owning
+	// the remastered reissue — the reissue should be reported missing.
+	seedArtist(t, db, "artist-1", "Rush")
+	seedLocalAlbum(t, db, "l1", "artist-1", "2112")
+	seedExternalRelease(t, db, "rg-standard", "artist-1", "2112", false)
+	seedExternalRelease(t, db, "rg-remaster", "artist-1", "2112 (Remastered)", false)
+
+	missing, err := ScanArtist(context.Background(), db, "artist-1", 0)
+	if err != nil {
+		t.Fatalf("ScanArtist() error: %v", err)
+	}
+
+	rgids := map[string]bool{}
+	for _, m := range missing {
+		rgids[m.RGID] = true
+	}
+	if rgids["rg-standard"] {
+		t.Errorf("standard 2112 should match local copy, not be missing")
+	}
+	if !rgids["rg-remaster"] {
+		t.Errorf("2112 (Remastered) reissue should be reported missing, got %v", rgids)
+	}
+}
+
 func TestScanArtist_CtxCancelled(t *testing.T) {
 	db := newTestDB(t)
 	defer db.Close()