diff --git a/cmd/naviwatcher/datastore_smoke_test.go b/cmd/naviwatcher/datastore_smoke_test.go index ad6ef45..c87a884 100644 --- a/cmd/naviwatcher/datastore_smoke_test.go +++ b/cmd/naviwatcher/datastore_smoke_test.go @@ -80,7 +80,7 @@ func TestDataFlowSmoke(t *testing.T) { }} tgCfg := config.TelegramConfig{Enabled: true} - n1, err := notifier.NotifyOnce(ctx, db, stub, tgCfg, "http://localhost:8080") + n1, err := notifier.NotifyOnce(ctx, db, stub, tgCfg, "http://localhost:8080", 0.85) if err != nil { t.Fatalf("NotifyOnce #1: %v", err) } @@ -91,7 +91,7 @@ func TestDataFlowSmoke(t *testing.T) { t.Fatalf("digest missing expected content: %v", sent) } - n2, err := notifier.NotifyOnce(ctx, db, stub, tgCfg, "http://localhost:8080") + n2, err := notifier.NotifyOnce(ctx, db, stub, tgCfg, "http://localhost:8080", 0.85) if err != nil { t.Fatalf("NotifyOnce #2: %v", err) } diff --git a/cmd/naviwatcher/main.go b/cmd/naviwatcher/main.go index e686f99..e4db985 100644 --- a/cmd/naviwatcher/main.go +++ b/cmd/naviwatcher/main.go @@ -214,9 +214,9 @@ func (a *App) startNotifier(ctx context.Context) { log.Printf("Notifier schedule invalid (%q): %v; notifier disabled", a.cfg.Telegram.CronSchedule, err) return } - uiBaseURL := a.cfg.Server.PublicURL + uiBaseURL := web.ResolveUIBaseURL(&a.cfg.Server) notifier.StartScheduler(ctx, true, schedule, func(ctx context.Context) error { - _, err := notifier.NotifyOnce(ctx, a.db, a.sender, a.cfg.Telegram, uiBaseURL) + _, err := notifier.NotifyOnce(ctx, a.db, a.sender, a.cfg.Telegram, uiBaseURL, a.cfg.Scanner.FuzzyThreshold) return err }, nil) } diff --git a/internal/navidrome/sync.go b/internal/navidrome/sync.go index 51b977d..5f450bf 100644 --- a/internal/navidrome/sync.go +++ b/internal/navidrome/sync.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log" "naviwatcher/internal/database" ) @@ -36,7 +37,11 @@ func SyncAlbums(ctx context.Context, client *NavidromeClient, db *database.DB) e albums, err := client.GetArtistAlbums(artist.ID) if err != nil { - return fmt.Errorf("sync albums: get albums for artist %s: %w", artist.ID, err) + // A transient failure for one artist must not abort the whole pull + // and take down the daemon; log it and continue with the remaining + // artists, matching the resilience of SyncAll/ScanAll. + log.Printf("sync albums: skip artist %s: %v", artist.ID, err) + continue } // Delete existing albums for this artist and insert fresh set within diff --git a/internal/navidrome/sync_test.go b/internal/navidrome/sync_test.go index e1ec68a..e0c0a71 100644 --- a/internal/navidrome/sync_test.go +++ b/internal/navidrome/sync_test.go @@ -360,18 +360,30 @@ func TestSyncAlbums_APIErrorMidSync(t *testing.T) { } ctx := context.Background() + // A transient failure for one artist must not abort the whole pull: the + // failing artist is skipped (and logged) while the others succeed. err = SyncAlbums(ctx, nc, db) - if err == nil { - t.Fatal("SyncAlbums() expected error for API failure mid-sync, got nil") + if err != nil { + t.Fatalf("SyncAlbums() expected no fatal error on per-artist API failure, got %v", err) } - // The first artist's albums should have been stored before the error. + // The first artist's albums should have been stored despite the second one + // failing. albums1, err := database.GetLocalAlbumsByArtist(db, "1") if err != nil { t.Fatalf("GetLocalAlbumsByArtist(1) error: %v", err) } if len(albums1) != 1 { - t.Errorf("expected 1 album for artist 1 (synced before error), got %d", len(albums1)) + t.Errorf("expected 1 album for artist 1 (synced before skip), got %d", len(albums1)) + } + + // The failing artist should have no local albums stored. + albums2, err := database.GetLocalAlbumsByArtist(db, "2") + if err != nil { + t.Fatalf("GetLocalAlbumsByArtist(2) error: %v", err) + } + if len(albums2) != 0 { + t.Errorf("expected 0 albums for artist 2 (skipped on error), got %d", len(albums2)) } } diff --git a/internal/notifier/scheduler.go b/internal/notifier/scheduler.go index 0bee925..7c03df9 100644 --- a/internal/notifier/scheduler.go +++ b/internal/notifier/scheduler.go @@ -11,71 +11,94 @@ import ( "naviwatcher/internal/scanner" ) -// NotifyOnce queries for releases that have not yet been notified, builds a -// digest, sends it through the given Sender, and marks each release as sent. -// Releases already present in notifications_sent are excluded upstream by -// GetUnnotifiedReleases, so this is idempotent across runs. +// NotifyOnce computes the releases that are genuinely missing for the user, +// intersects that set with the releases not yet notified, builds a digest of +// the result, sends it through the given Sender, and marks each release as +// sent. // -// If there are no unnotified releases nothing is sent (the caller's scheduler -// is responsible for not spamming the operator with an empty digest). When -// releases are present, each is marked sent so a subsequent run will not -// re-notify it. +// "Missing" is the authoritative definition produced by the scanner: an +// external release with no sufficiently similar local album (see +// scanner.ScanAll). This intersection is what keeps the digest honest: a +// release the user already owns in Navidrome must never be reported as a new +// missing release, even though it still counts as "unnotified" on a fresh +// database. Releases already in notifications_sent are excluded by +// GetUnnotifiedReleases, so the call is idempotent across runs. +// +// threshold is the fuzzy-similarity cutoff passed through to the scanner; 0 +// selects the scanner's default. It must match config.Scanner.FuzzyThreshold +// so the digest honors the operator's configured tolerance. +// +// If there are no newly-missing releases nothing is sent (the caller's +// scheduler is responsible for not spamming the operator with an empty +// digest). When releases are present, each is marked sent so a subsequent run +// will not re-notify it. // // uiBaseURL is the externally-reachable base URL of the Web UI, appended to the // digest so operators can jump to the dashboard. -func NotifyOnce(ctx context.Context, db *database.DB, sender Sender, cfg config.TelegramConfig, uiBaseURL string) (int, error) { +func NotifyOnce(ctx context.Context, db *database.DB, sender Sender, cfg config.TelegramConfig, uiBaseURL string, threshold float64) (int, error) { if sender == nil { return 0, fmt.Errorf("notifier: sender must not be nil") } + // Authoritative missing set: external releases with no matching local album. + // This is what the Web UI dashboard also shows, so the digest stays + // consistent with what the operator sees in the UI. + missing, err := scanner.ScanAll(ctx, db, threshold) + if err != nil { + return 0, fmt.Errorf("notifier: scan missing releases: %w", err) + } + missingByRGID := make(map[string]scanner.MissingRelease, len(missing)) + for _, m := range missing { + missingByRGID[m.RGID] = m + } + + // Restrict to releases not yet notified. A release that is genuinely missing + // but was already announced is dropped here so it is never re-sent. unnotified, err := database.GetUnnotifiedReleases(db) if err != nil { return 0, fmt.Errorf("notifier: query unnotified releases: %w", err) } - missing := make([]scanner.MissingRelease, 0, len(unnotified)) + toNotify := make([]scanner.MissingRelease, 0, len(unnotified)) for _, r := range unnotified { - missing = append(missing, scanner.MissingRelease{ - RGID: r.RGID, - ArtistID: r.ArtistID, - Title: r.Title, - Type: r.Type, - ReleaseDate: r.ReleaseDate, - }) + if m, ok := missingByRGID[r.RGID]; ok { + toNotify = append(toNotify, m) + } } // Resolve human-readable artist names so the digest shows recognizable // labels instead of opaque internal artist IDs. A lookup failure for a // single artist must not abort the whole digest, so errors are ignored and // that artist falls back to its ID via artistLabel. - names := make(map[string]string, len(missing)) - for _, r := range unnotified { - if _, ok := names[r.ArtistID]; ok { + names := make(map[string]string, len(toNotify)) + for _, m := range toNotify { + if _, ok := names[m.ArtistID]; ok { continue } - settings, err := database.GetArtistSettings(db, r.ArtistID) + settings, err := database.GetArtistSettings(db, m.ArtistID) if err == nil && settings.Name != "" { - names[r.ArtistID] = settings.Name + names[m.ArtistID] = settings.Name } } - message := FormatDigest(missing, uiBaseURL, names) // Nothing to report: skip sending so the operator is not spammed with an // empty digest on every cron fire. The startup fire likewise stays quiet // until the first genuinely missing release appears. - if len(unnotified) == 0 { + if len(toNotify) == 0 { return 0, nil } + + message := FormatDigest(toNotify, uiBaseURL, names) if err := sender.Send(ctx, message); err != nil { return 0, fmt.Errorf("notifier: send digest: %w", err) } - for _, r := range unnotified { - if err := database.MarkNotificationSent(db, r.RGID); err != nil { - return 0, fmt.Errorf("notifier: mark sent for %s: %w", r.RGID, err) + for _, m := range toNotify { + if err := database.MarkNotificationSent(db, m.RGID); err != nil { + return 0, fmt.Errorf("notifier: mark sent for %s: %w", m.RGID, err) } } - return len(unnotified), nil + return len(toNotify), nil } // Schedule produces the next firing time strictly after the given time. It diff --git a/internal/notifier/scheduler_test.go b/internal/notifier/scheduler_test.go index 7f7c6c7..6b5efc7 100644 --- a/internal/notifier/scheduler_test.go +++ b/internal/notifier/scheduler_test.go @@ -23,9 +23,9 @@ func (f fixedSchedule) Next(t time.Time) time.Time { // collectSender records messages and can be told to fail. type collectSender struct { - mu sync.Mutex + mu sync.Mutex messages []string - failErr error + failErr error } func (s *collectSender) Send(ctx context.Context, message string) error { @@ -79,7 +79,7 @@ func TestNotifyOnce_SendsAndMarksSent(t *testing.T) { sender := &collectSender{} cfg := config.TelegramConfig{Enabled: true} - n, err := NotifyOnce(context.Background(), db, sender, cfg, "http://ui:8080") + n, err := NotifyOnce(context.Background(), db, sender, cfg, "http://ui:8080", 0.85) if err != nil { t.Fatalf("NotifyOnce: %v", err) } @@ -100,6 +100,48 @@ func TestNotifyOnce_SendsAndMarksSent(t *testing.T) { } } +func TestNotifyOnce_OwnedReleaseNotNotified(t *testing.T) { + // Regression: a cached external release the user already owns locally must + // not be reported as "missing". Before the scanner intersection was added, + // NotifyOnce treated every unnotified external release as missing and would + // spam the operator with releases they already have in Navidrome. + db, err := database.New(":memory:") + if err != nil { + t.Fatalf("New(): %v", err) + } + defer db.Close() + + seedRelease(t, db, "rgid-owned", "artist-1", false) + // The user already has this album locally, with a matching normalized title. + if _, err := db.Conn().Exec( + "INSERT INTO local_albums (id, artist_id, title) VALUES (?, ?, ?)", + "local-1", "artist-1", "Release rgid-owned", + ); err != nil { + t.Fatalf("seed local album: %v", err) + } + + sender := &collectSender{} + n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui", 0.85) + if err != nil { + t.Fatalf("NotifyOnce: %v", err) + } + if n != 0 { + t.Fatalf("expected 0 notified (release already owned), got %d", n) + } + if sender.count() != 0 { + t.Fatalf("expected no digest for an owned release, got %d message(s)", sender.count()) + } + // The owned release is genuinely missing per the scanner, so it must remain + // un-marked-sent to avoid corrupting notifications_sent state. + sent, err := database.IsNotificationSent(db, "rgid-owned") + if err != nil { + t.Fatalf("IsNotificationSent: %v", err) + } + if sent { + t.Error("owned release should NOT be marked sent") + } +} + func TestNotifyOnce_EmptyDoesNotSend(t *testing.T) { db, err := database.New(":memory:") if err != nil { @@ -108,7 +150,7 @@ func TestNotifyOnce_EmptyDoesNotSend(t *testing.T) { defer db.Close() sender := &collectSender{} - n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui") + n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui", 0.85) if err != nil { t.Fatalf("NotifyOnce: %v", err) } @@ -132,7 +174,7 @@ func TestNotifyOnce_SkipsAlreadySent(t *testing.T) { seedRelease(t, db, "rgid-new", "artist-1", false) sender := &collectSender{} - n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui") + n, err := NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui", 0.85) if err != nil { t.Fatalf("NotifyOnce: %v", err) } @@ -168,7 +210,7 @@ func TestNotifyOnce_SendErrorNotMarked(t *testing.T) { want := errors.New("send boom") sender := &collectSender{failErr: want} - _, err = NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui") + _, err = NotifyOnce(context.Background(), db, sender, config.TelegramConfig{}, "http://ui", 0.85) if err == nil || !errors.Is(err, want) { t.Fatalf("expected error %v, got %v", want, err) } @@ -189,7 +231,7 @@ func TestNotifyOnce_NilSender(t *testing.T) { } defer db.Close() - if _, err := NotifyOnce(context.Background(), db, nil, config.TelegramConfig{}, "http://ui"); err == nil { + if _, err := NotifyOnce(context.Background(), db, nil, config.TelegramConfig{}, "http://ui", 0.85); err == nil { t.Fatal("expected error for nil sender") } } diff --git a/internal/web/handlers.go b/internal/web/handlers.go index 9e246c7..7db839b 100644 --- a/internal/web/handlers.go +++ b/internal/web/handlers.go @@ -7,12 +7,25 @@ import ( "fmt" "html/template" "net/http" + "regexp" "naviwatcher/internal/config" "naviwatcher/internal/database" "naviwatcher/internal/scanner" ) +// idPattern bounds the {id} path segment accepted by artist routes. Artist IDs +// come from Navidrome (numeric/UUID) and MusicBrainz (UUID), so word +// characters and hyphens cover every legitimate value. Rejecting anything else +// prevents a crafted id (containing "/", control characters, or whitespace) +// from breaking route matching or being reflected into a Location header. +var idPattern = regexp.MustCompile(`^[\w-]+$`) + +// isValidID reports whether s is a safe artist-ID path segment. +func isValidID(s string) bool { + return idPattern.MatchString(s) +} + //go:embed templates/*.html var templates embed.FS @@ -84,7 +97,7 @@ func (s *Server) handleArtist(w http.ResponseWriter, r *http.Request) { // The enhanced ServeMux extracts the {id} path segment for us. id := r.PathValue("id") - if id == "" { + if id == "" || !isValidID(id) { http.NotFound(w, r) return } @@ -156,7 +169,7 @@ func (s *Server) buildArtistData(ctx context.Context, id string) (*ArtistData, e // ArchiveData is the view model for the ignored-releases archive page. type ArchiveData struct { - Releases []MissingReleaseView + Releases []MissingReleaseView UIBaseURL string } @@ -196,15 +209,27 @@ func (s *Server) handleArchive(w http.ResponseWriter, r *http.Request) { // *config.Config (mirroring how the app constructs other components). It // forwards the server sub-config and derives uiBaseURL from the configured // public_url, falling back to a best-effort host:port. +// ResolveUIBaseURL derives the externally-reachable base URL of the Web UI from +// config. An explicit public_url (e.g. behind a reverse proxy) is preferred. +// When unset, it falls back to http://host:port — unless the bind host is the +// unspecified "0.0.0.0" (not reachable from outside the host), in which case an +// empty string is returned so callers omit the link rather than advertise an +// unusable address. The same derivation is used by both the Web UI and the +// Telegram notifier so dashboard links are consistent across surfaces. +func ResolveUIBaseURL(cfg *config.ServerConfig) string { + base := cfg.PublicURL + if base == "" && cfg.Host != "0.0.0.0" && cfg.Host != "" { + base = fmt.Sprintf("http://%s:%d", cfg.Host, cfg.Port) + } + return base +} + func NewServerWithConfig(cfg *config.Config, db *database.DB) *Server { // Prefer an explicit, externally-reachable public_url (e.g. behind a // reverse proxy). Fall back to host:port — but if the bind host is the // unspecified "0.0.0.0", it is not reachable from outside the host, so // omit the link rather than advertise an unusable address. - base := cfg.Server.PublicURL - if base == "" && cfg.Server.Host != "0.0.0.0" && cfg.Server.Host != "" { - base = fmt.Sprintf("http://%s:%d", cfg.Server.Host, cfg.Server.Port) - } + base := ResolveUIBaseURL(&cfg.Server) return NewServer(&cfg.Server, db, base, cfg.Scanner.FuzzyThreshold) } @@ -219,7 +244,7 @@ func (s *Server) ignoreOrRestore(w http.ResponseWriter, r *http.Request) { return } id := r.PathValue("id") - if id == "" { + if id == "" || !isValidID(id) { http.NotFound(w, r) return } @@ -270,7 +295,7 @@ func (s *Server) toggleIgnoreSingles(w http.ResponseWriter, r *http.Request) { return } id := r.PathValue("id") - if id == "" { + if id == "" || !isValidID(id) { http.NotFound(w, r) return } @@ -289,4 +314,3 @@ func (s *Server) toggleIgnoreSingles(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/artist/"+id, http.StatusSeeOther) } -