diff --git a/cmd/naviwatcher/main.go b/cmd/naviwatcher/main.go index 24d2c82..e619a64 100644 --- a/cmd/naviwatcher/main.go +++ b/cmd/naviwatcher/main.go @@ -131,17 +131,14 @@ 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. - if err := a.doSync(ctx); err != nil { - if ctx.Err() != nil { - return nil - } - 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. + // Start the Web UI dashboard FIRST, in its own goroutine, so the dashboard + // accepts connections immediately. The initial sync below is throttled by + // the MusicBrainz 1 req/s limit and can take many minutes on a large + // library (worst case: a fresh DB where every artist needs MBID + // resolution) — exactly when an operator is most likely watching. Starting + // the server first means the dashboard is reachable (serving cached data) + // during that window instead of refusing connections. It serves until ctx + // is cancelled, then shuts down gracefully. if a.web != nil { go func() { if err := a.web.Start(ctx); err != nil { @@ -157,7 +154,10 @@ func (a *App) run(ctx context.Context) error { // disabled (sender nil / enabled false), so always calling it is safe. a.startNotifier(ctx) - // Kick off the periodic sync+scan loop goroutine. + // Kick off the periodic sync+scan loop. It runs an immediate first sync + // (governed by the same overlap guard as periodic ticks) so the service + // produces results without waiting a full interval, without racing a + // concurrent tick over the shared DB and rate-limited MusicBrainz client. a.startPeriodicSync(ctx) <-ctx.Done() @@ -237,25 +237,38 @@ func (a *App) startPeriodicSync(ctx context.Context) { var free = make(chan struct{}, 1) free <- struct{}{} + // launch starts a guarded sync if the slot is free, returning true when a + // sync was started and false when one is already in progress. The in-flight + // goroutine returns the token when done. + launch := func(label string) bool { + select { + case <-free: + go func() { + defer func() { free <- struct{}{} }() + if err := a.doSync(ctx); err != nil { + if ctx.Err() != nil { + return + } + log.Printf("%s sync+scan failed: %v", label, err) + } + }() + return true + default: + return false + } + } + + // Immediate first sync (guarded), so the service produces results without + // waiting a full interval and without racing the first ticker fire. + launch("Initial") + for { select { case <-ctx.Done(): log.Println("Periodic sync stopped.") return case <-ticker.C: - 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) - } - }() - default: + if !launch("Periodic") { // Previous sync still running; skip this tick. log.Println("Skipping periodic sync: previous sync still in progress.") } diff --git a/cmd/naviwatcher/main_test.go b/cmd/naviwatcher/main_test.go index a44063e..324190f 100644 --- a/cmd/naviwatcher/main_test.go +++ b/cmd/naviwatcher/main_test.go @@ -143,8 +143,10 @@ func TestStartPeriodicSync_CancelsCleanly(t *testing.T) { close(done) }() - // With a 1h interval the ticker would never fire on its own; cancel should - // return promptly. + // With a 1h interval the ticker never fires on its own; cancel should + // return promptly. The loop does run one immediate (guarded) sync at + // startup, so depending on scheduling calls may be 0 (cancel won the race) + // or 1 (immediate sync ran) — but never more, since no tick can fire in 1h. cancel() select { @@ -154,8 +156,8 @@ func TestStartPeriodicSync_CancelsCleanly(t *testing.T) { t.Fatal("startPeriodicSync did not exit after ctx cancellation") } - if got := atomic.LoadInt64(&calls); got != 0 { - t.Errorf("expected no sync calls with 1h interval, got %d", got) + if got := atomic.LoadInt64(&calls); got > 1 { + t.Errorf("expected at most 1 (immediate) sync call with 1h interval, got %d", got) } } diff --git a/internal/database/external_releases.go b/internal/database/external_releases.go index 4f1e32f..926a6cf 100644 --- a/internal/database/external_releases.go +++ b/internal/database/external_releases.go @@ -52,7 +52,7 @@ func GetExternalRelease(db *DB, rgid string) (*ExternalRelease, error) { var cachedAt sql.NullTime var secondaryTypes sql.NullString err := db.Conn().QueryRow( - "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE rgid = ?", + "SELECT rgid, artist_id, title, COALESCE(type,''), COALESCE(release_date,''), is_ignored, cached_at, secondary_types FROM external_releases WHERE rgid = ?", rgid, ).Scan(&r.RGID, &r.ArtistID, &r.Title, &r.Type, &r.ReleaseDate, &r.IsIgnored, &cachedAt, &secondaryTypes) if err != nil { @@ -83,7 +83,7 @@ func SaveExternalRelease(db *DB, release *ExternalRelease) error { // GetExternalReleasesByArtist returns all external_release rows for a given artist_id. func GetExternalReleasesByArtist(db *DB, artistID string) ([]ExternalRelease, error) { rows, err := db.Conn().Query( - "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE artist_id = ?", + "SELECT rgid, artist_id, title, COALESCE(type,''), COALESCE(release_date,''), is_ignored, cached_at, secondary_types FROM external_releases WHERE artist_id = ?", artistID, ) if err != nil { @@ -116,7 +116,7 @@ func GetExternalReleasesByArtist(db *DB, artistID string) ([]ExternalRelease, er // GetIgnoredReleases returns all external_release rows where is_ignored = 1. func GetIgnoredReleases(db *DB) ([]ExternalRelease, error) { rows, err := db.Conn().Query( - "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE is_ignored = 1", + "SELECT rgid, artist_id, title, COALESCE(type,''), COALESCE(release_date,''), is_ignored, cached_at, secondary_types FROM external_releases WHERE is_ignored = 1", ) if err != nil { return nil, fmt.Errorf("query ignored releases: %w", err) diff --git a/internal/database/notifications.go b/internal/database/notifications.go index a650e06..d3abc15 100644 --- a/internal/database/notifications.go +++ b/internal/database/notifications.go @@ -1,16 +1,37 @@ package database import ( + "errors" "fmt" + + sqlite3 "github.com/mattn/go-sqlite3" ) // MarkNotificationSent records that a notification has been sent for the given RGID. +// +// Uses INSERT OR IGNORE so a pre-existing marker for the same RGID (a +// same-second re-notify colliding on the (rgid, sent_at) primary key) is a +// no-op rather than an error: the marker's presence, not its exact timestamp, +// is what matters for idempotency. +// +// A concurrent re-sync that prunes the external_releases row before this insert +// would violate the FK constraint. OR IGNORE does NOT downgrade FK violations +// in this SQLite build, so the FK error is caught explicitly and treated as a +// benign no-op ("the release is already gone"). This ensures a single vanished +// release cannot abort a whole digest's mark-sent loop and trigger duplicate +// notifications on the next run. func MarkNotificationSent(db *DB, rgid string) error { _, err := db.Conn().Exec( - "INSERT INTO notifications_sent (rgid) VALUES (?)", + "INSERT OR IGNORE INTO notifications_sent (rgid) VALUES (?)", rgid, ) if err != nil { + var sqliteErr sqlite3.Error + if errors.As(err, &sqliteErr) && sqliteErr.Code == sqlite3.ErrConstraint && + sqliteErr.ExtendedCode == sqlite3.ErrConstraintForeignKey { + // Release row was pruned concurrently; nothing to mark. + return nil + } return fmt.Errorf("mark notification sent: %w", err) } return nil @@ -33,7 +54,7 @@ func IsNotificationSent(db *DB, rgid string) (bool, error) { // artists are excluded so the digest honors the monitoring contract. func GetUnnotifiedReleases(db *DB) ([]ExternalRelease, error) { rows, err := db.Conn().Query(` - SELECT e.rgid, e.artist_id, e.title, e.type, e.release_date, e.is_ignored + SELECT e.rgid, e.artist_id, e.title, COALESCE(e.type,''), COALESCE(e.release_date,''), e.is_ignored FROM external_releases e JOIN artist_settings s ON e.artist_id = s.id LEFT JOIN notifications_sent n ON e.rgid = n.rgid diff --git a/internal/database/notifications_test.go b/internal/database/notifications_test.go index a5f87e0..75524b1 100644 --- a/internal/database/notifications_test.go +++ b/internal/database/notifications_test.go @@ -41,9 +41,38 @@ func TestMarkNotificationSent_New(t *testing.T) { } } -// TestMarkNotificationSent_DuplicateSecond verifies that inserting the same RGID twice -// within the same second fails due to the composite primary key (rgid, sent_at). -// In practice, notifications are sent at most once per day, so this is acceptable. +// TestMarkNotificationSent_MissingReleaseIsNoOp verifies that marking a release +// whose external_releases row does not exist (e.g. pruned by a concurrent +// re-sync) does not error: the FK violation is swallowed by INSERT OR IGNORE so +// a single vanished release cannot abort a digest's mark-sent loop. +func TestMarkNotificationSent_MissingReleaseIsNoOp(t *testing.T) { + db, err := New(":memory:") + if err != nil { + t.Fatalf("New() error: %v", err) + } + defer db.Close() + + // No artist/release inserted: rgid-gone has no external_releases row. + if err := MarkNotificationSent(db, "rgid-gone"); err != nil { + t.Fatalf("MarkNotificationSent() for missing release should be a no-op, got error: %v", err) + } + + // Nothing should have been recorded (FK violation ignored, row skipped). + var count int + if err := db.Conn().QueryRow("SELECT COUNT(*) FROM notifications_sent WHERE rgid = ?", "rgid-gone").Scan(&count); err != nil { + t.Fatalf("count query error: %v", err) + } + if count != 0 { + t.Errorf("expected 0 notification rows for missing release, got %d", count) + } +} + +// TestMarkNotificationSent_DuplicateSecond verifies that marking the same RGID +// twice within the same second is an idempotent no-op (INSERT OR IGNORE) rather +// than an error: a same-second collision on the composite primary key +// (rgid, sent_at) must not abort a digest's mark-sent loop, since that would +// leave later releases unmarked and cause duplicate notifications on the next +// run. The marker's presence, not its exact timestamp, is what matters. func TestMarkNotificationSent_DuplicateSecond(t *testing.T) { db, err := New(":memory:") if err != nil { @@ -60,10 +89,9 @@ func TestMarkNotificationSent_DuplicateSecond(t *testing.T) { if err := MarkNotificationSent(db, "rgid-1"); err != nil { t.Fatalf("first MarkNotificationSent() error: %v", err) } - // Second insert in the same second should fail with a UNIQUE constraint error. - err = MarkNotificationSent(db, "rgid-1") - if err == nil { - t.Fatal("expected UNIQUE constraint error on duplicate insert, got nil") + // Second mark in the same second should be a silent no-op, not an error. + if err := MarkNotificationSent(db, "rgid-1"); err != nil { + t.Fatalf("duplicate MarkNotificationSent() should be a no-op, got error: %v", err) } // Should still have exactly one row. diff --git a/internal/notifier/scheduler.go b/internal/notifier/scheduler.go index dc5d476..c3af317 100644 --- a/internal/notifier/scheduler.go +++ b/internal/notifier/scheduler.go @@ -93,9 +93,13 @@ func NotifyOnce(ctx context.Context, db *database.DB, sender Sender, cfg config. return 0, fmt.Errorf("notifier: send digest: %w", err) } + // The digest has already been delivered at this point. A failure to mark a + // single release must NOT abort the loop: doing so would leave later + // releases unmarked and cause them to be re-notified (duplicate digest) on + // the next run. Log and continue so every release in this batch is marked. 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) + log.Printf("notifier: mark sent for %s failed: %v", m.RGID, err) } } return len(toNotify), nil