diff --git a/README.md b/README.md index a5b8b26..bea96a3 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,11 @@ server: port: 8080 username: "admin" password: "CHANGE_ME" + # Externally-reachable base URL for links in Telegram digests (e.g. behind a + # reverse proxy). If omitted, links are derived from host:port — but when host + # is 0.0.0.0 (the unspecified bind address) no link is emitted, since it is + # not reachable from outside the host. + public_url: "https://naviwatcher.example.com" navidrome: url: "http://localhost:4533" @@ -203,6 +208,10 @@ server: port: 8080 username: "admin" password: "CHANGE_ME" + # Внешний базовый URL для ссылок в дайджестах Telegram (напр. за обратным прокси). + # Если не задан, ссылки строятся из host:port — но при host 0.0.0.0 (несpecificированный + # адрес привязки) ссылка не генерируется, так как недоступна снаружи хоста. + public_url: "https://naviwatcher.example.com" navidrome: url: "http://localhost:4533" diff --git a/cmd/naviwatcher/datastore_smoke_test.go b/cmd/naviwatcher/datastore_smoke_test.go index 765d555..ad6ef45 100644 --- a/cmd/naviwatcher/datastore_smoke_test.go +++ b/cmd/naviwatcher/datastore_smoke_test.go @@ -106,7 +106,7 @@ func TestDataFlowSmoke(t *testing.T) { Username: "admin", Password: "secret", } - srv := web.NewServer(srvCfg, db, "http://localhost:8080") + srv := web.NewServer(srvCfg, db, "http://localhost:8080", 0) // Unauthenticated -> 401. rec := httptest.NewRecorder() diff --git a/cmd/naviwatcher/main.go b/cmd/naviwatcher/main.go index e93ce15..e686f99 100644 --- a/cmd/naviwatcher/main.go +++ b/cmd/naviwatcher/main.go @@ -14,7 +14,9 @@ import ( "naviwatcher/internal/database" "naviwatcher/internal/musicbrainz" "naviwatcher/internal/navidrome" + "naviwatcher/internal/notifier" "naviwatcher/internal/scanner" + "naviwatcher/internal/web" ) // App holds all application dependencies for clean shutdown and testability. @@ -23,6 +25,8 @@ type App struct { db *database.DB mbClient *musicbrainz.MusicBrainzClient ndClient *navidrome.NavidromeClient + web *web.Server + sender notifier.Sender // syncFn, when non-nil, replaces the real syncAndScan call in // startPeriodicSync so tests can observe the loop without live clients. @@ -94,11 +98,23 @@ func NewApp(ctx context.Context, cfg *config.Config, dbPath string) (*App, error return nil, fmt.Errorf("failed to initialize navidrome client: %w", err) } + // Build the Web UI dashboard server (not started until run). + webServer := web.NewServerWithConfig(cfg, db) + + // Build the notifier sender. A nil sender is fine when Telegram is disabled; + // the scheduler is no-op-safe and the web UI needs no sender. + var sender notifier.Sender + if cfg.Telegram.Enabled { + sender = notifier.NewTelegramSender(cfg.Telegram) + } + return &App{ cfg: cfg, db: db, mbClient: mbClient, ndClient: ndClient, + web: webServer, + sender: sender, }, nil } @@ -107,10 +123,6 @@ func (a *App) Close() { if a.mbClient != nil { a.mbClient.Close() } - if a.ndClient != nil { - // NavidromeClient holds a stateless subsonic client; nothing to close - // beyond releasing idle connections tracked by the MusicBrainz client. - } if a.db != nil { if err := a.db.Close(); err != nil { log.Printf("Error closing database: %v", err) @@ -120,9 +132,7 @@ func (a *App) Close() { func (a *App) run(ctx context.Context) error { // Run an immediate sync+scan so the service produces results without - // waiting a full interval, then kick off the periodic loop goroutine. - // Business logic added in later tasks (notifier, web server) will be - // wired as additional goroutines below. + // waiting a full interval. if err := a.doSync(ctx); err != nil { if ctx.Err() != nil { return nil @@ -130,6 +140,24 @@ func (a *App) run(ctx context.Context) error { log.Printf("Initial sync+scan failed: %v", err) } + // Start the Web UI dashboard in its own goroutine; it serves until ctx is + // cancelled, then shuts down gracefully. + if a.web != nil { + go func() { + if err := a.web.Start(ctx); err != nil { + if ctx.Err() != nil { + return + } + log.Printf("Web UI server stopped with error: %v", err) + } + }() + } + + // Start the Telegram notifier scheduler. It is no-op-safe when Telegram is + // disabled (sender nil / enabled false), so always calling it is safe. + a.startNotifier(ctx) + + // Kick off the periodic sync+scan loop goroutine. a.startPeriodicSync(ctx) <-ctx.Done() @@ -173,28 +201,62 @@ func (a *App) syncAndScan(ctx context.Context) error { return nil } +// startNotifier wires the Telegram digest scheduler. The scheduler is +// no-op-safe (returns without starting when disabled or sender is nil), so it +// is always safe to call. The base URL for dashboard links comes from the +// server's configured public_url. +func (a *App) startNotifier(ctx context.Context) { + if !a.cfg.Telegram.Enabled { + return + } + schedule, err := notifier.NewCronSchedule(a.cfg.Telegram.CronSchedule) + if err != nil { + log.Printf("Notifier schedule invalid (%q): %v; notifier disabled", a.cfg.Telegram.CronSchedule, err) + return + } + uiBaseURL := a.cfg.Server.PublicURL + notifier.StartScheduler(ctx, true, schedule, func(ctx context.Context) error { + _, err := notifier.NotifyOnce(ctx, a.db, a.sender, a.cfg.Telegram, uiBaseURL) + return err + }, nil) +} + // startPeriodicSync runs syncAndScan on a ticker at cfg.Sync.Interval. It -// blocks until ctx is cancelled, then returns cleanly. Each tick runs in its -// own goroutine so a slow sync does not block the ticker; a fresh interval is -// still scheduled regardless. +// blocks until ctx is cancelled, then returns cleanly. Each tick spawns a +// goroutine so a slow sync does not block the ticker, but a new sync is +// skipped while the previous one is still running (guarded by a done channel) +// so syncs never overlap and contend for the shared DB and rate-limited +// MusicBrainz client. func (a *App) startPeriodicSync(ctx context.Context) { ticker := time.NewTicker(a.cfg.Sync.Interval) defer ticker.Stop() + // free is a sentinel channel: nil means a sync is currently in flight. + var free = make(chan struct{}, 1) + free <- struct{}{} + for { select { case <-ctx.Done(): log.Println("Periodic sync stopped.") return case <-ticker.C: - go func() { - if err := a.doSync(ctx); err != nil { - if ctx.Err() != nil { - return + select { + case <-free: + // Slot was free; start a sync and release the slot when done. + go func() { + defer func() { free <- struct{}{} }() + if err := a.doSync(ctx); err != nil { + if ctx.Err() != nil { + return + } + log.Printf("Periodic sync+scan failed: %v", err) } - log.Printf("Periodic sync+scan failed: %v", err) - } - }() + }() + default: + // Previous sync still running; skip this tick. + log.Println("Skipping periodic sync: previous sync still in progress.") + } } } } diff --git a/config.yaml.example b/config.yaml.example index 2c70eee..a745e9f 100644 --- a/config.yaml.example +++ b/config.yaml.example @@ -4,6 +4,11 @@ server: # Basic Auth for Web UI access username: "admin" password: "CHANGE_ME" + # Externally-reachable base URL for links in Telegram digests (e.g. behind a + # reverse proxy). If omitted, links are derived from host:port — but when host + # is 0.0.0.0 (the unspecified bind address) no link is emitted, since it is + # not reachable from outside the host. + public_url: "https://naviwatcher.example.com" navidrome: url: "http://localhost:4533" diff --git a/docs/plans/2026-07-19-notifier-webui-sync.md b/docs/plans/2026-07-19-notifier-webui-sync.md index 2686198..c721cc0 100644 --- a/docs/plans/2026-07-19-notifier-webui-sync.md +++ b/docs/plans/2026-07-19-notifier-webui-sync.md @@ -161,3 +161,30 @@ name collisions.) ignore/restore actions persist. - **External**: ensure `config.yaml.example` matches deployed config; Telegram bot token/chat_id must be supplied by operator. + +## Follow-up fixes (post code review) +After the plan's Tasks 1-10 merged, a code review surfaced and fixed: +- **Duplicate notifications**: `SyncArtistDiscography` previously deleted + `notifications_sent` for the whole artist on every cache-miss re-sync, which + wiped "already notified" tracking and re-sent digests. Now only notifications + for releases that disappear are pruned, and surviving releases keep their sent + markers. Re-sync is FK-safe (uses `INSERT OR REPLACE` + `rgid NOT IN (…)`). +- **Empty-discography caching**: artists with zero MusicBrainz release groups + were never cached (a `0`-row result was treated as a cache miss), re-fetching + every cycle. Added `artist_settings.last_synced` (migration `009`) as the cache + freshness signal so empty discographies honor the TTL. +- **`main.run` wiring**: the Web UI server and Telegram notifier scheduler are + now constructed in `NewApp` and started as goroutines in `run()` (previously + only the sync loop ran). +- **Overlap guard**: `startPeriodicSync` now skips a tick while a previous sync + is still in flight (buffered `done` channel) so syncs never overlap. +- **`server.public_url` config**: added so Telegram digest links use an + externally-reachable origin instead of the bind `host:port` (which defaults to + `0.0.0.0`). `NewServerWithConfig` falls back to host:port only for a real host. +- **Web handlers**: artist detail page now uses `scanner.ScanArtist` (per-artist) + instead of a full `ScanAll`; removed the always-false `releaseIgnored` lookup + and dead `endsWith` helper; the configured fuzzy threshold is now threaded + through `Server`. +- **DB connection pooling**: `:memory:` databases now use `SetMaxOpenConns(1)` + so migrations and queries share one in-memory store (prevents "missing column" + errors under the connection pool). diff --git a/internal/config/config.go b/internal/config/config.go index aaa0621..824e318 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -20,10 +20,11 @@ type Config struct { // ServerConfig holds HTTP server settings. type ServerConfig struct { - Host string `yaml:"host"` - Port int `yaml:"port"` - Username string `yaml:"username"` - Password string `yaml:"password"` + Host string `yaml:"host"` + Port int `yaml:"port"` + Username string `yaml:"username"` + Password string `yaml:"password"` + PublicURL string `yaml:"public_url"` } // NavidromeConfig holds Subsonic API connection details. diff --git a/internal/database/artist_settings.go b/internal/database/artist_settings.go index 96665e5..6162fc3 100644 --- a/internal/database/artist_settings.go +++ b/internal/database/artist_settings.go @@ -4,19 +4,27 @@ import ( "database/sql" "errors" "fmt" + "time" ) +// DBer is the minimal query interface satisfied by both *sql.DB and *sql.Tx, +// so callers can run statements inside or outside a transaction. +type DBer interface { + Exec(query string, args ...interface{}) (sql.Result, error) +} + // GetArtistSettings retrieves an artist_settings row by ID. // Returns sql.ErrNoRows if the artist is not found. func GetArtistSettings(db *DB, id string) (*ArtistSettings, error) { var ( - s ArtistSettings - mbid sql.NullString + s ArtistSettings + mbid sql.NullString + lastSynced sql.NullTime ) err := db.Conn().QueryRow( - "SELECT id, name, mbid, ignore_singles, ignore_compilations, monitored FROM artist_settings WHERE id = ?", + "SELECT id, name, mbid, ignore_singles, ignore_compilations, monitored, last_synced FROM artist_settings WHERE id = ?", id, - ).Scan(&s.ID, &s.Name, &mbid, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored) + ).Scan(&s.ID, &s.Name, &mbid, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored, &lastSynced) if err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, ErrArtistNotFound @@ -24,9 +32,26 @@ func GetArtistSettings(db *DB, id string) (*ArtistSettings, error) { return nil, err } s.MBID = mbid.String + if lastSynced.Valid { + s.LastSynced = lastSynced.Time + } return &s, nil } +// TouchArtistSynced records that the artist was synced at the given time. It +// is used by the MusicBrainz pipeline to mark a successful sync (even one that +// found zero release groups) so the cache TTL is honoured. +func TouchArtistSynced(db DBer, artistID string, syncedAt time.Time) error { + _, err := db.Exec( + "UPDATE artist_settings SET last_synced = ? WHERE id = ?", + FormatCachedAt(syncedAt), artistID, + ) + if err != nil { + return fmt.Errorf("touch artist synced: %w", err) + } + return nil +} + // SaveArtistSettings inserts or replaces an artist_settings row. func SaveArtistSettings(db *DB, settings *ArtistSettings) error { _, err := db.Conn().Exec( diff --git a/internal/database/database.go b/internal/database/database.go index 8af7f8e..ec0777e 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -25,10 +25,21 @@ func New(dbPath string) (*DB, error) { // 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. + // A plain ":memory:" database is private to the connection that opened it, + // so a pool of N connections would give N separate empty databases and + // migrations would appear missing on some. Limiting the pool to a single + // connection keeps one in-memory database per New() call, which is correct + // for both tests (isolated) and the single-process production service. conn, err := sql.Open("sqlite3", dbPath+"?_foreign_keys=on") if err != nil { return nil, fmt.Errorf("open database: %w", err) } + if dbPath == ":memory:" { + conn.SetMaxOpenConns(1) + } + if err != nil { + return nil, fmt.Errorf("open database: %w", err) + } // Enable WAL mode for better concurrent read performance. if _, err := conn.Exec("PRAGMA journal_mode=WAL"); err != nil { @@ -135,6 +146,10 @@ func (db *DB) migrate() error { name: "008_add_mbid_to_artist_settings", sql: `ALTER TABLE artist_settings ADD COLUMN mbid TEXT;`, }, + { + name: "009_add_last_synced_to_artist_settings", + sql: `ALTER TABLE artist_settings ADD COLUMN last_synced DATETIME;`, + }, } for _, m := range migrations { @@ -181,12 +196,13 @@ func (db *DB) isMigrationApplied(name string) (bool, error) { // ArtistSettings represents a row in the artist_settings table. type ArtistSettings struct { - ID string `json:"id"` - Name string `json:"name"` - MBID string `json:"mbid"` - IgnoreSingles bool `json:"ignore_singles"` - IgnoreCompilations bool `json:"ignore_compilations"` - Monitored bool `json:"monitored"` + ID string `json:"id"` + Name string `json:"name"` + MBID string `json:"mbid"` + IgnoreSingles bool `json:"ignore_singles"` + IgnoreCompilations bool `json:"ignore_compilations"` + Monitored bool `json:"monitored"` + LastSynced time.Time `json:"last_synced"` } // LocalAlbum represents a row in the local_albums table. diff --git a/internal/database/database_test.go b/internal/database/database_test.go index 67d74b5..4a1bcc6 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -205,11 +205,11 @@ func TestMigrationTracking(t *testing.T) { t.Fatalf("query migrations count: %v", err) } - // We have 8 recorded migrations: artist_settings, external_releases, + // We have 9 recorded migrations: artist_settings, external_releases, // local_albums, notifications_sent, cached_at column, secondary_types - // column, the external_releases.artist_id index, and the artist_settings - // mbid column. - if count != 8 { - t.Errorf("expected 8 applied migrations, got %d", count) + // column, the external_releases.artist_id index, the artist_settings mbid + // column, and the artist_settings last_synced column. + if count != 9 { + t.Errorf("expected 9 applied migrations, got %d", count) } } diff --git a/internal/database/external_releases.go b/internal/database/external_releases.go index 2de2fae..e7d1e63 100644 --- a/internal/database/external_releases.go +++ b/internal/database/external_releases.go @@ -166,6 +166,42 @@ func SetReleaseIgnored(db *DB, rgid string, ignored bool) error { return nil } +// ArtistCacheFresh reports whether the artist was synced within the TTL. A hit +// requires a fresh external_releases row (covers pre-seeded/non-empty caches) +// OR a fresh artist_settings.last_synced (covers empty discographies, which +// store no external_releases rows but are still marked as synced). Either +// signal means we should not re-fetch from MusicBrainz. +func ArtistCacheFresh(db *DB, artistID string, ttl time.Duration) (bool, error) { + if ttl <= 0 { + return false, nil + } + cutoff := time.Now().UTC().Add(-ttl).Format(utcLayout) + var dummy int + err := db.Conn().QueryRow( + "SELECT 1 FROM external_releases WHERE artist_id = ? AND cached_at >= ? LIMIT 1", + artistID, cutoff, + ).Scan(&dummy) + if err == nil { + return true, nil + } + if err != sql.ErrNoRows { + return false, fmt.Errorf("check artist cache freshness (releases): %w", err) + } + + // Fall back to the per-artist last_synced marker (set even on empty syncs). + err = db.Conn().QueryRow( + "SELECT 1 FROM artist_settings WHERE id = ? AND last_synced >= ? LIMIT 1", + artistID, cutoff, + ).Scan(&dummy) + if err == nil { + return true, nil + } + if err == sql.ErrNoRows { + return false, nil + } + return false, fmt.Errorf("check artist cache freshness (settings): %w", err) +} + // 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) { diff --git a/internal/musicbrainz/sync.go b/internal/musicbrainz/sync.go index 5f79e38..dcbd951 100644 --- a/internal/musicbrainz/sync.go +++ b/internal/musicbrainz/sync.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "fmt" + "strings" "time" "naviwatcher/internal/database" @@ -38,18 +39,25 @@ func SyncArtistDiscography( return nil, fmt.Errorf("sync artist discography: %w", err) } - // Step 1: Check cache. + // Step 1: Check cache freshness. A genuine hit means the artist was synced + // within the TTL — even when it has zero release groups. We must not gate on + // row count, or artists with an empty MusicBrainz discography would be + // re-fetched on every sync (defeating the TTL and wasting the 1 req/s budget). cachedReleases, err := database.GetExternalReleasesByArtistWithCache(db, artistID, ttl) if err != nil { return nil, fmt.Errorf("sync artist discography: cache check failed: %w", err) } + fresh, err := database.ArtistCacheFresh(db, artistID, ttl) + if err != nil { + return nil, fmt.Errorf("sync artist discography: cache freshness check failed: %w", err) + } - // Step 2: If we have cached data, return it. Re-apply the per-artist type - // toggles even on a cache hit so user changes to ignore_singles / - // ignore_compilations take effect without waiting for cache expiry. - // (Status/type inclusion was already applied when the rows were first + // Step 2: If we have a fresh cache, return the cached data. Re-apply the + // per-artist type toggles even on a cache hit so user changes to + // ignore_singles / ignore_compilations take effect without waiting for cache + // expiry. (Status/type inclusion was already applied when the rows were first // synced and stored, so only the toggles can change.) - if len(cachedReleases) > 0 { + if fresh { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("sync artist discography: %w", err) } @@ -108,17 +116,44 @@ func SyncArtistDiscography( } rows.Close() - // Delete old entries for this artist to avoid stale records. - // Must delete notifications_sent first to avoid FK violation since - // notifications_sent.rgid references external_releases.rgid. - if _, err := tx.Exec( - "DELETE FROM notifications_sent WHERE rgid IN (SELECT rgid FROM external_releases WHERE artist_id = ?)", - artistID, - ); err != nil { - return nil, fmt.Errorf("sync artist discography: delete old notifications: %w", err) + // Build the set of RGIDs present in this sync so we can drop only the rows + // that disappeared, leaving the rest (and their notification markers) intact. + synced := make([]any, 0, len(filtered)) + for _, rg := range filtered { + synced = append(synced, rg.ID) } - if _, err := tx.Exec("DELETE FROM external_releases WHERE artist_id = ?", artistID); err != nil { - return nil, fmt.Errorf("sync artist discography: delete old releases: %w", err) + + // Drop notification markers for releases that are gone. This runs before the + // external_releases delete so the FK on notifications_sent.rgid stays valid + // (we only ever delete from notifications_sent here). + if len(synced) > 0 { + placeholders := strings.Repeat("?,", len(synced)) + placeholders = placeholders[:len(placeholders)-1] + query := fmt.Sprintf( + "DELETE FROM notifications_sent WHERE rgid IN (SELECT rgid FROM external_releases WHERE artist_id = ? AND rgid NOT IN (%s))", + placeholders, + ) + args := append([]any{artistID}, synced...) + if _, err := tx.Exec(query, args...); err != nil { + return nil, fmt.Errorf("sync artist discography: prune stale notifications: %w", err) + } + // Remove external_release rows that are no longer part of the discography. + delQuery := fmt.Sprintf( + "DELETE FROM external_releases WHERE artist_id = ? AND rgid NOT IN (%s)", + placeholders, + ) + if _, err := tx.Exec(delQuery, args...); err != nil { + return nil, fmt.Errorf("sync artist discography: delete stale releases: %w", err) + } + } else { + // No releases this sync: the artist may have an empty discography. Drop + // everything we previously cached for them. + if _, err := tx.Exec("DELETE FROM notifications_sent WHERE rgid IN (SELECT rgid FROM external_releases WHERE artist_id = ?)", artistID); err != nil { + return nil, fmt.Errorf("sync artist discography: delete notifications: %w", err) + } + if _, err := tx.Exec("DELETE FROM external_releases WHERE artist_id = ?", artistID); err != nil { + return nil, fmt.Errorf("sync artist discography: delete old releases: %w", err) + } } var releases []database.ExternalRelease @@ -136,7 +171,7 @@ func SyncArtistDiscography( } if _, err := tx.Exec( - "INSERT INTO external_releases (rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + "INSERT OR REPLACE INTO external_releases (rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", ext.RGID, ext.ArtistID, ext.Title, ext.Type, ext.ReleaseDate, ext.IsIgnored, database.FormatCachedAt(ext.CachedAt), database.JoinSecondaryTypes(ext.SecondaryTypes), ); err != nil { return nil, fmt.Errorf("sync artist discography: insert release %s: %w", rg.ID, err) @@ -145,6 +180,13 @@ func SyncArtistDiscography( releases = append(releases, *ext) } + // Mark the artist as synced (even when it has zero release groups) so the + // cache TTL honours empty discographies and they are not re-fetched every + // cycle. + if err := database.TouchArtistSynced(tx, artistID, now); err != nil { + return nil, fmt.Errorf("sync artist discography: touch last_synced: %w", err) + } + if err := tx.Commit(); err != nil { return nil, fmt.Errorf("sync artist discography: commit transaction: %w", err) } diff --git a/internal/musicbrainz/sync_test.go b/internal/musicbrainz/sync_test.go index d472239..1c8aff8 100644 --- a/internal/musicbrainz/sync_test.go +++ b/internal/musicbrainz/sync_test.go @@ -370,11 +370,16 @@ func TestSyncArtistDiscography_IdempotentResync(t *testing.T) { t.Fatalf("expected 1 server call after first sync, got %d", callCount) } - // Force cache expiry by setting cached_at to the past. + // Force cache expiry by setting cached_at (on external_releases) and + // last_synced (on artist_settings) to the past. _, err = db.Conn().Exec("UPDATE external_releases SET cached_at = ? WHERE artist_id = ?", time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID) if err != nil { - t.Fatalf("expire cache: %v", err) + t.Fatalf("expire cache (releases): %v", err) + } + if _, err = db.Conn().Exec("UPDATE artist_settings SET last_synced = ? WHERE id = ?", + time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID); err != nil { + t.Fatalf("expire cache (settings): %v", err) } // Second sync should re-fetch from API (cache expired). @@ -438,6 +443,28 @@ func TestSyncArtistDiscography_EmptyResponse(t *testing.T) { if len(stored) != 0 { t.Errorf("expected 0 stored releases, got %d", len(stored)) } + + // A second sync within the TTL must be a cache hit: an empty discography is + // now cached via artist_settings.last_synced, so the MusicBrainz API must + // not be re-queried (and still returns 0 releases). + serverHits := 0 + server2 := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { + serverHits++ + w.Header().Set("Content-Type", "application/xml") + w.Write([]byte(mbReleaseGroupListResponse("", 0))) + }) + defer server2.Close() + + releases2, err := SyncArtistDiscography(ctx, newTestClient(server2.URL), db, artistID, artistMBID, ttl) + if err != nil { + t.Fatalf("second SyncArtistDiscography() error: %v", err) + } + if len(releases2) != 0 { + t.Errorf("expected 0 releases on cached empty sync, got %d", len(releases2)) + } + if serverHits != 0 { + t.Errorf("expected empty discography to be cached (0 API calls), got %d", serverHits) + } } // ----------------------------------------------------------------------- @@ -714,11 +741,16 @@ func TestSyncArtistDiscography_CleansStaleReleases(t *testing.T) { t.Fatalf("expected 3 releases after first sync, got %d", len(releases1)) } - // Force cache expiry by setting cached_at to the past. + // Force cache expiry by setting cached_at (on external_releases) and + // last_synced (on artist_settings) to the past. _, err = db.Conn().Exec("UPDATE external_releases SET cached_at = ? WHERE artist_id = ?", time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID) if err != nil { - t.Fatalf("expire cache: %v", err) + t.Fatalf("expire cache (releases): %v", err) + } + if _, err = db.Conn().Exec("UPDATE artist_settings SET last_synced = ? WHERE id = ?", + time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID); err != nil { + t.Fatalf("expire cache (settings): %v", err) } // Second sync should re-fetch from API (cache expired). @@ -876,10 +908,14 @@ func TestSyncArtistDiscography_ResyncWithNotifications(t *testing.T) { artistID := "nav-fk-test" artistName := "FK Artist" + // First response includes two release groups; the second sync drops one + // ("rg-2") so we can verify its notification is pruned while the surviving + // release's notification ("rg-1") is preserved. server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { resp := mbReleaseGroupListResponse( - mbReleaseGroupXML("rg-1", "Album", "Album", "Official", artistMBID, artistName, "2024-01-01"), - 1, + mbReleaseGroupXML("rg-1", "Album", "Album", "Official", artistMBID, artistName, "2024-01-01")+ + mbReleaseGroupXML("rg-2", "Album", "Album", "Official", artistMBID, artistName, "2023-01-01"), + 2, ) w.Write([]byte(resp)) }) @@ -893,27 +929,37 @@ func TestSyncArtistDiscography_ResyncWithNotifications(t *testing.T) { ctx := context.Background() // First sync. - _, err := SyncArtistDiscography(ctx, client, db, artistID, artistMBID, 24*time.Hour) - if err != nil { + if _, err := SyncArtistDiscography(ctx, client, db, artistID, artistMBID, 24*time.Hour); err != nil { t.Fatalf("first SyncArtistDiscography() error: %v", err) } - // Insert a notifications_sent row referencing the release. - _, err = db.Conn().Exec( - "INSERT INTO notifications_sent (rgid) VALUES (?)", "rg-1", - ) - if err != nil { - t.Fatalf("insert notification: %v", err) + // Mark both releases as already notified. + for _, rgid := range []string{"rg-1", "rg-2"} { + if _, err := db.Conn().Exec("INSERT INTO notifications_sent (rgid) VALUES (?)", rgid); err != nil { + t.Fatalf("insert notification: %v", err) + } } - // Force cache expiry. - _, err = db.Conn().Exec("UPDATE external_releases SET cached_at = ? WHERE artist_id = ?", - time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID) - if err != nil { - t.Fatalf("expire cache: %v", err) + // Force cache expiry on the first sync so the second sync re-fetches. + if _, err := db.Conn().Exec("UPDATE external_releases SET cached_at = ? WHERE artist_id = ?", + time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID); err != nil { + t.Fatalf("expire cache (releases): %v", err) + } + if _, err := db.Conn().Exec("UPDATE artist_settings SET last_synced = ? WHERE id = ?", + time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID); err != nil { + t.Fatalf("expire cache (settings): %v", err) } - // Second sync should succeed without FK violation. + // Second sync returns only rg-1 (drop rg-2 from the server response) and + // must succeed without an FK violation. + server.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + resp := mbReleaseGroupListResponse( + mbReleaseGroupXML("rg-1", "Album", "Album", "Official", artistMBID, artistName, "2024-01-01"), + 1, + ) + w.Write([]byte(resp)) + }) + releases, err := SyncArtistDiscography(ctx, client, db, artistID, artistMBID, 24*time.Hour) if err != nil { t.Fatalf("second SyncArtistDiscography() error (FK violation?): %v", err) @@ -922,13 +968,20 @@ func TestSyncArtistDiscography_ResyncWithNotifications(t *testing.T) { t.Fatalf("expected 1 release after resync, got %d", len(releases)) } - // Notification should have been cleaned up. - var count int - err = db.Conn().QueryRow("SELECT COUNT(*) FROM notifications_sent WHERE rgid = ?", "rg-1").Scan(&count) - if err != nil { - t.Fatalf("count notifications: %v", err) + // The surviving release's notification must be preserved (no duplicate + // digest on the next notify run). The dropped release's notification must + // be pruned. + var count1, count2 int + if err := db.Conn().QueryRow("SELECT COUNT(*) FROM notifications_sent WHERE rgid = ?", "rg-1").Scan(&count1); err != nil { + t.Fatalf("count rg-1 notifications: %v", err) } - if count != 0 { - t.Errorf("expected 0 notifications after resync, got %d", count) + if err := db.Conn().QueryRow("SELECT COUNT(*) FROM notifications_sent WHERE rgid = ?", "rg-2").Scan(&count2); err != nil { + t.Fatalf("count rg-2 notifications: %v", err) + } + if count1 != 1 { + t.Errorf("expected surviving release rg-1 notification preserved (1), got %d", count1) + } + if count2 != 0 { + t.Errorf("expected dropped release rg-2 notification pruned (0), got %d", count2) } } diff --git a/internal/web/handlers.go b/internal/web/handlers.go index 3a41c4d..92bfda2 100644 --- a/internal/web/handlers.go +++ b/internal/web/handlers.go @@ -32,7 +32,7 @@ func (s *Server) handleDashboard(w http.ResponseWriter, r *http.Request) { return } - threshold := s.defaultThreshold() + threshold := s.threshold data, err := s.buildDashboardData(r.Context(), threshold) if err != nil { http.Error(w, fmt.Sprintf("failed to build dashboard: %v", err), http.StatusInternalServerError) @@ -120,11 +120,14 @@ func (s *Server) buildArtistData(ctx context.Context, id string) (*ArtistData, e return nil, fmt.Errorf("load local albums: %w", err) } - // Compute the missing releases for this artist. - threshold := s.defaultThreshold() - missing, err := scanner.ScanAll(ctx, s.db, threshold) + // Compute the missing releases for this single artist (ScanArtist scopes the + // query to the artist instead of scanning every monitored artist). ScanAll + // excludes ignored releases, so every missing release surfaced here is, by + // definition, not ignored. + threshold := s.threshold + missing, err := scanner.ScanArtist(ctx, s.db, id, threshold) if err != nil { - return nil, fmt.Errorf("scan: %w", err) + return nil, fmt.Errorf("scan artist: %w", err) } data := &ArtistData{ @@ -138,38 +141,18 @@ func (s *Server) buildArtistData(ctx context.Context, id string) (*ArtistData, e data.LocalAlbums = append(data.LocalAlbums, LocalAlbumView{Title: a.Title}) } for _, m := range missing { - if m.ArtistID != id { - continue - } - ignored, igErr := s.releaseIgnored(id, m.RGID) - if igErr != nil { - return nil, igErr - } data.Missing = append(data.Missing, MissingReleaseView{ - ArtistID: id, + ArtistID: m.ArtistID, RGID: m.RGID, Title: m.Title, Type: m.Type, ReleaseDate: m.ReleaseDate, - Ignored: ignored, + Ignored: false, }) } return data, nil } -// releaseIgnored reports whether the external release with the given RGID is -// flagged ignored. -func (s *Server) releaseIgnored(artistID, rgid string) (bool, error) { - // ScanAll already excludes ignored releases, so a missing release shown here - // is, by definition, not ignored. We still surface the persisted flag so the - // UI can reflect a release that was ignored and later re-evaluated. - rel, err := database.GetExternalRelease(s.db, rgid) - if err != nil { - return false, fmt.Errorf("get external release %s: %w", rgid, err) - } - return rel.IsIgnored, nil -} - // ArchiveData is the view model for the ignored-releases archive page. type ArchiveData struct { Releases []MissingReleaseView @@ -208,20 +191,20 @@ func (s *Server) handleArchive(w http.ResponseWriter, r *http.Request) { } } -// defaultThreshold returns the fuzzy threshold to use when scanning for the -// dashboard. It is currently fixed at the engine default; later wiring can -// thread the configured threshold through the Server if desired. -func (s *Server) defaultThreshold() float64 { - return 0 // 0 → scanner.DefaultThreshold -} - // NewServerWithConfig is a convenience constructor that accepts the full // *config.Config (mirroring how the app constructs other components). It -// forwards the server sub-config and derives uiBaseURL from host/port. +// forwards the server sub-config and derives uiBaseURL from the configured +// public_url, falling back to a best-effort host:port. func NewServerWithConfig(cfg *config.Config, db *database.DB) *Server { - // Build a best-effort external base URL from the server config. - base := fmt.Sprintf("http://%s:%d", cfg.Server.Host, cfg.Server.Port) - return NewServer(&cfg.Server, db, base) + // 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) + } + return NewServer(&cfg.Server, db, base, cfg.Scanner.FuzzyThreshold) } // ignoreOrRestore handles the POST /artist/{id}/ignore and .../restore routes. @@ -290,8 +273,3 @@ func (s *Server) toggleIgnoreSingles(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/artist/"+id, http.StatusSeeOther) } -// endsWith reports whether s ends with suffix. -func endsWith(s, suffix string) bool { - return len(s) >= len(suffix) && s[len(s)-len(suffix):] == suffix -} - diff --git a/internal/web/server.go b/internal/web/server.go index 5d50a4a..4cc70ac 100644 --- a/internal/web/server.go +++ b/internal/web/server.go @@ -27,16 +27,22 @@ type Server struct { // uiBaseURL is the externally reachable base URL of the dashboard (scheme + // host), used to build links in notifications and elsewhere. Optional. uiBaseURL string + + // threshold is the fuzzy-similarity cutoff used when scanning for missing + // releases; 0 means use the scanner default. + threshold float64 } // NewServer constructs a dashboard Server bound to the given DB and server // config. uiBaseURL is the externally reachable origin (e.g. // "http://localhost:8080") used when rendering absolute links; pass "" to omit. -func NewServer(cfg *config.ServerConfig, db *database.DB, uiBaseURL string) *Server { +// threshold is the fuzzy-similarity cutoff (0 → scanner default). +func NewServer(cfg *config.ServerConfig, db *database.DB, uiBaseURL string, threshold float64) *Server { s := &Server{ cfg: cfg, db: db, uiBaseURL: strings.TrimRight(uiBaseURL, "/"), + threshold: threshold, } mux := http.NewServeMux() mux.HandleFunc("/", s.handleDashboard) diff --git a/internal/web/server_test.go b/internal/web/server_test.go index 1f82059..4dfcc14 100644 --- a/internal/web/server_test.go +++ b/internal/web/server_test.go @@ -51,7 +51,7 @@ func newServer(t *testing.T, user, pass string) (*Server, *database.DB) { t.Fatalf("open db: %v", err) } cfg := &config.ServerConfig{Host: "0.0.0.0", Port: 8080, Username: user, Password: pass} - s := NewServer(cfg, db, "http://ui.example") + s := NewServer(cfg, db, "http://ui.example", 0) return s, db }