feat: add SyncAll periodic sync pipeline and wire Navidrome client into App

cmd/naviwatcher/main.go

@@ -12,6 +12,7 @@ import (
 	"naviwatcher/internal/config"
 	"naviwatcher/internal/database"
 	"naviwatcher/internal/musicbrainz"
+	"naviwatcher/internal/navidrome"
 	"naviwatcher/internal/scanner"
 )
 
@@ -20,8 +21,14 @@ type App struct {
 	cfg      *config.Config
 	db       *database.DB
 	mbClient *musicbrainz.MusicBrainzClient
+	ndClient *navidrome.NavidromeClient
 }
 
+// navidromeClientFactory constructs the Navidrome client. It is a package-level
+// variable (not a direct call to navidrome.NewClient) so tests can inject a stub
+// without requiring a live Navidrome server for authentication.
+var navidromeClientFactory = navidrome.NewClient
+
 const defaultConfigPath = "config.yaml"
 
 func main() {
@@ -76,10 +83,17 @@ func NewApp(ctx context.Context, cfg *config.Config, dbPath string) (*App, error
 
 	log.Printf("MusicBrainz client initialized (user-agent: %s)", cfg.MusicBrainz.UserAgent)
 
+	// Initialize Navidrome client (authenticates immediately; error if auth fails).
+	ndClient, err := navidromeClientFactory(cfg.Navidrome)
+	if err != nil {
+		return nil, fmt.Errorf("failed to initialize navidrome client: %w", err)
+	}
+
 	return &App{
 		cfg:      cfg,
 		db:       db,
 		mbClient: mbClient,
+		ndClient: ndClient,
 	}, nil
 }
 
@@ -88,6 +102,10 @@ 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)

cmd/naviwatcher/main_test.go

@@ -12,6 +12,7 @@ import (
 
 	"naviwatcher/internal/config"
 	"naviwatcher/internal/database"
+	"naviwatcher/internal/navidrome"
 )
 
 func TestAppRun_ScanLogsMissingReleases(t *testing.T) {
@@ -135,6 +136,13 @@ func TestNewApp_CreatesMusicBrainzClient(t *testing.T) {
 		},
 	}
 
+	// Inject an unauthenticated Navidrome client so the test needs no live server.
+	prevFactory := navidromeClientFactory
+	navidromeClientFactory = func(c config.NavidromeConfig) (*navidrome.NavidromeClient, error) {
+		return navidrome.NewClientUnauthenticated(c), nil
+	}
+	defer func() { navidromeClientFactory = prevFactory }()
+
 	ctx := context.Background()
 	app, err := NewApp(ctx, cfg, ":memory:")
 	if err != nil {
@@ -145,6 +153,9 @@ func TestNewApp_CreatesMusicBrainzClient(t *testing.T) {
 	if app.mbClient == nil {
 		t.Fatal("expected MusicBrainz client to be initialized, got nil")
 	}
+	if app.ndClient == nil {
+		t.Fatal("expected Navidrome client to be initialized, got nil")
+	}
 	if app.db == nil {
 		t.Fatal("expected database to be initialized, got nil")
 	}
@@ -170,6 +181,12 @@ func TestNewApp_GracefulShutdown(t *testing.T) {
 		},
 	}
 
+	prevFactory := navidromeClientFactory
+	navidromeClientFactory = func(c config.NavidromeConfig) (*navidrome.NavidromeClient, error) {
+		return navidrome.NewClientUnauthenticated(c), nil
+	}
+	defer func() { navidromeClientFactory = prevFactory }()
+
 	ctx := context.Background()
 	app, err := NewApp(ctx, cfg, ":memory:")
 	if err != nil {
@@ -197,6 +214,12 @@ func TestAppRun_GracefulShutdown(t *testing.T) {
 		},
 	}
 
+	prevFactory := navidromeClientFactory
+	navidromeClientFactory = func(c config.NavidromeConfig) (*navidrome.NavidromeClient, error) {
+		return navidrome.NewClientUnauthenticated(c), nil
+	}
+	defer func() { navidromeClientFactory = prevFactory }()
+
 	ctx, cancel := context.WithCancel(context.Background())
 
 	app, err := NewApp(ctx, cfg, ":memory:")

docs/plans/2026-07-19-notifier-webui-sync.md

@@ -95,10 +95,10 @@ name collisions.)
 - [x] run tests - must pass before task 3
 
 ### Task 3: Periodic sync pipeline
-- [ ] add `SyncAll(ctx, ndClient, mbClient, db, ttl)` orchestrator: for each monitored artist → ensure MBID (resolve + persist if missing) → `musicbrainz.SyncArtistDiscography` → `navidrome.SyncAlbums`
-- [ ] wire `navidrome.NewClient` into `App`; add `ndClient` field
-- [ ] write tests for `SyncAll` with stubbed clients + `:memory:` DB (new artist gets MBID, existing MBID reused, unmonitored skipped)
-- [ ] run tests - must pass before task 4
+- [x] add `SyncAll(ctx, ndClient, mbClient, db, ttl)` orchestrator: for each monitored artist → ensure MBID (resolve + persist if missing) → `musicbrainz.SyncArtistDiscography` → `navidrome.SyncAlbums`
+- [x] wire `navidrome.NewClient` into `App`; add `ndClient` field
+- [x] write tests for `SyncAll` with stubbed clients + `:memory:` DB (new artist gets MBID, existing MBID reused, unmonitored skipped)
+- [x] run tests - must pass before task 4
 
 ### Task 4: Main loop wiring (sync → scan)
 - [ ] replace compute-only `run()` with: one immediate sync+scan, then a ticker-driven periodic sync+scan goroutine; keep graceful shutdown via ctx

internal/musicbrainz/syncall.go

@@ -0,0 +1,141 @@
+package musicbrainz
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	"naviwatcher/internal/database"
+)
+
+// MBIDResolver resolves a MusicBrainz artist ID for an artist name.
+// The real *MusicBrainzClient satisfies this interface.
+type MBIDResolver interface {
+	ResolveArtistMBID(ctx context.Context, name string) (string, error)
+}
+
+// ArtistDiscographySyncer syncs one artist's MusicBrainz discography into the
+// external_releases table. The real implementation (musicbrainz.SyncArtistDiscography)
+// is wrapped by discographySyncer so the concrete *MusicBrainzClient dependency
+// is injectable in tests.
+type ArtistDiscographySyncer interface {
+	SyncArtistDiscography(ctx context.Context, db *database.DB, artistID, artistMBID string, ttl time.Duration) ([]database.ExternalRelease, error)
+}
+
+// discographySyncer adapts the package-level SyncArtistDiscography function to
+// the ArtistDiscographySyncer interface, binding a concrete *MusicBrainzClient.
+type discographySyncer struct {
+	client *MusicBrainzClient
+}
+
+// NewDiscographySyncer wraps a *MusicBrainzClient as an ArtistDiscographySyncer.
+func NewDiscographySyncer(client *MusicBrainzClient) ArtistDiscographySyncer {
+	return &discographySyncer{client: client}
+}
+
+func (s *discographySyncer) SyncArtistDiscography(
+	ctx context.Context,
+	db *database.DB,
+	artistID, artistMBID string,
+	ttl time.Duration,
+) ([]database.ExternalRelease, error) {
+	return SyncArtistDiscography(ctx, s.client, db, artistID, artistMBID, ttl)
+}
+
+// AlbumSyncer copies each monitored artist's albums from Navidrome into the
+// local_albums table. The real implementation (navidrome.SyncAlbums) is wrapped
+// so the concrete *navidrome.NavidromeClient dependency is injectable in tests.
+type AlbumSyncer interface {
+	SyncAlbums(ctx context.Context, db *database.DB) error
+}
+
+// albumSyncer adapts navidrome.SyncAlbums to the AlbumSyncer interface.
+type albumSyncer struct {
+	syncAlbums func(ctx context.Context, db *database.DB) error
+}
+
+func (s *albumSyncer) SyncAlbums(ctx context.Context, db *database.DB) error {
+	return s.syncAlbums(ctx, db)
+}
+
+// SyncAll orchestrates the data pipeline for every monitored artist:
+//  1. MusicBrainz artist-ID resolution — for each artist with no cached MBID,
+//     resolve it by name and persist it on the artist_settings row. Artists that
+//     already have an MBID reuse it (no extra rate-limited MusicBrainz call).
+//  2. MusicBrainz discography sync into external_releases.
+//  3. Navidrome album sync into local_albums.
+//
+// Ordering matters: Navidrome's artist/album tables are populated by the caller
+// before SyncAll (via navidrome.SyncArtists / SyncAlbums as appropriate); here we
+// focus on the per-artist MBID + discography + album refresh. Unmonitored artists
+// are skipped.
+//
+// Resolution failures for a single artist are logged and skipped (the artist is
+// left for the next sync) rather than aborting the whole run; the error is still
+// returned so the caller can decide whether to surface it.
+func SyncAll(
+	ctx context.Context,
+	db *database.DB,
+	resolver MBIDResolver,
+	discography ArtistDiscographySyncer,
+	albums AlbumSyncer,
+	ttl time.Duration,
+) error {
+	if err := ctx.Err(); err != nil {
+		return fmt.Errorf("sync all: %w", err)
+	}
+
+	artists, err := database.GetAllArtistSettings(db)
+	if err != nil {
+		return fmt.Errorf("sync all: get artists: %w", err)
+	}
+
+	var resolutionErr error
+	for _, artist := range artists {
+		if err := ctx.Err(); err != nil {
+			return fmt.Errorf("sync all: %w", err)
+		}
+
+		// Skip unmonitored artists entirely.
+		if !artist.Monitored {
+			continue
+		}
+
+		// Ensure we have an MBID; resolve and persist if missing.
+		mbid := artist.MBID
+		if mbid == "" {
+			resolved, rerr := resolver.ResolveArtistMBID(ctx, artist.Name)
+			if rerr != nil {
+				// Skip this artist but remember the first resolution error.
+				if resolutionErr == nil {
+					resolutionErr = fmt.Errorf("resolve MBID for artist %q: %w", artist.Name, rerr)
+				}
+				continue
+			}
+			mbid = resolved
+			if perr := database.UpdateArtistSettings(db, artist.ID, map[string]interface{}{"mbid": mbid}); perr != nil {
+				if resolutionErr == nil {
+					resolutionErr = fmt.Errorf("persist MBID for artist %q: %w", artist.Name, perr)
+				}
+				continue
+			}
+		}
+
+		// Sync the artist's MusicBrainz discography.
+		if _, derr := discography.SyncArtistDiscography(ctx, db, artist.ID, mbid, ttl); derr != nil {
+			if resolutionErr == nil {
+				resolutionErr = fmt.Errorf("sync discography for artist %q: %w", artist.Name, derr)
+			}
+			continue
+		}
+	}
+
+	// Album sync operates over all monitored artists in one pass.
+	if aerr := albums.SyncAlbums(ctx, db); aerr != nil {
+		if resolutionErr == nil {
+			resolutionErr = fmt.Errorf("sync albums: %w", aerr)
+		}
+	}
+
+	return resolutionErr
+}

internal/musicbrainz/syncall_test.go

@@ -0,0 +1,218 @@
+package musicbrainz
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"testing"
+	"time"
+
+	"naviwatcher/internal/database"
+)
+
+// stubResolver is a configurable MBIDResolver for tests.
+type stubResolver struct {
+	byName map[string]string // name -> mbid
+	calls  []string          // names requested, in order
+	err    error             // optional error to return for any resolve
+}
+
+func (s *stubResolver) ResolveArtistMBID(ctx context.Context, name string) (string, error) {
+	s.calls = append(s.calls, name)
+	if s.err != nil {
+		return "", s.err
+	}
+	if mbid, ok := s.byName[name]; ok {
+		return mbid, nil
+	}
+	return "", errors.New("no match")
+}
+
+// stubDiscography records per-artist discography syncs.
+type stubDiscography struct {
+	synced []string // artistIDs
+	err    error
+}
+
+func (s *stubDiscography) SyncArtistDiscography(
+	ctx context.Context,
+	db *database.DB,
+	artistID, artistMBID string,
+	ttl time.Duration,
+) ([]database.ExternalRelease, error) {
+	if s.err != nil {
+		return nil, s.err
+	}
+	s.synced = append(s.synced, artistID)
+	return nil, nil
+}
+
+// stubAlbums records album-sync invocations.
+type stubAlbums struct {
+	called int
+	err    error
+}
+
+func (s *stubAlbums) SyncAlbums(ctx context.Context, db *database.DB) error {
+	s.called++
+	return s.err
+}
+
+func seedArtistRow(t *testing.T, db *database.DB, id, name, mbid string, monitored bool) {
+	t.Helper()
+	if err := database.SaveArtistSettings(db, &database.ArtistSettings{
+		ID:        id,
+		Name:      name,
+		MBID:      mbid,
+		Monitored: monitored,
+	}); err != nil {
+		t.Fatalf("seed artist: %v", err)
+	}
+}
+
+func TestSyncAll_NewArtistGetsMBID(t *testing.T) {
+	db := newTestDB(t)
+	seedArtistRow(t, db, "ar1", "Radiohead", "", true)
+
+	resolver := &stubResolver{byName: map[string]string{"Radiohead": "mbid-radiohead"}}
+	disco := &stubDiscography{}
+	albs := &stubAlbums{}
+
+	err := SyncAll(context.Background(), db, resolver, disco, albs, 24*time.Hour)
+	if err != nil {
+		t.Fatalf("SyncAll() error = %v", err)
+	}
+
+	// Resolver must have been called for the new artist.
+	if len(resolver.calls) != 1 || resolver.calls[0] != "Radiohead" {
+		t.Fatalf("resolver calls = %v, want [Radiohead]", resolver.calls)
+	}
+	// MBID persisted on the row.
+	got, gerr := database.GetArtistSettings(db, "ar1")
+	if gerr != nil {
+		t.Fatalf("GetArtistSettings() error = %v", gerr)
+	}
+	if got.MBID != "mbid-radiohead" {
+		t.Errorf("persisted MBID = %q, want %q", got.MBID, "mbid-radiohead")
+	}
+	// Discography and albums synced.
+	if len(disco.synced) != 1 || disco.synced[0] != "ar1" {
+		t.Errorf("discography synced = %v, want [ar1]", disco.synced)
+	}
+	if albc := albs.called; albc != 1 {
+		t.Errorf("albums sync called = %d, want 1", albc)
+	}
+}
+
+func TestSyncAll_ExistingMBIDReused(t *testing.T) {
+	db := newTestDB(t)
+	seedArtistRow(t, db, "ar1", "Radiohead", "preset-mbid", true)
+
+	resolver := &stubResolver{byName: map[string]string{"Radiohead": "resolved-mbid"}}
+	disco := &stubDiscography{}
+	albs := &stubAlbums{}
+
+	if err := SyncAll(context.Background(), db, resolver, disco, albs, 24*time.Hour); err != nil {
+		t.Fatalf("SyncAll() error = %v", err)
+	}
+
+	// Resolver must NOT be called when MBID already present.
+	if len(resolver.calls) != 0 {
+		t.Errorf("resolver calls = %v, want none (MBID reused)", resolver.calls)
+	}
+	got, _ := database.GetArtistSettings(db, "ar1")
+	if got.MBID != "preset-mbid" {
+		t.Errorf("MBID = %q, want preserved preset-mbid", got.MBID)
+	}
+	if len(disco.synced) != 1 {
+		t.Errorf("discography synced = %v, want [ar1]", disco.synced)
+	}
+}
+
+func TestSyncAll_UnmonitoredSkipped(t *testing.T) {
+	db := newTestDB(t)
+	seedArtistRow(t, db, "ar1", "Radiohead", "", false) // unmonitored
+
+	resolver := &stubResolver{byName: map[string]string{"Radiohead": "mbid-x"}}
+	disco := &stubDiscography{}
+	albs := &stubAlbums{}
+
+	if err := SyncAll(context.Background(), db, resolver, disco, albs, 24*time.Hour); err != nil {
+		t.Fatalf("SyncAll() error = %v", err)
+	}
+
+	if len(resolver.calls) != 0 {
+		t.Errorf("resolver calls = %v, want none (unmonitored skipped)", resolver.calls)
+	}
+	if len(disco.synced) != 0 {
+		t.Errorf("discography synced = %v, want none", disco.synced)
+	}
+	// Album sync still runs (it internally skips unmonitored too), but no
+	// discography work should have happened for the skipped artist.
+}
+
+func TestSyncAll_ResolutionErrorSkipsArtist(t *testing.T) {
+	db := newTestDB(t)
+	seedArtistRow(t, db, "ar1", "Unknown", "", true)
+
+	resolver := &stubResolver{err: errors.New("mb down")}
+	disco := &stubDiscography{}
+	albs := &stubAlbums{}
+
+	err := SyncAll(context.Background(), db, resolver, disco, albs, 24*time.Hour)
+	if err == nil {
+		t.Fatal("SyncAll() expected error when resolution fails")
+	}
+	if len(disco.synced) != 0 {
+		t.Errorf("discography synced = %v, want none (resolution failed)", disco.synced)
+	}
+	// MBID must remain empty since persistence was skipped.
+	got, _ := database.GetArtistSettings(db, "ar1")
+	if got.MBID != "" {
+		t.Errorf("MBID = %q, want empty after failed resolution", got.MBID)
+	}
+}
+
+func TestSyncAll_ContextCancel(t *testing.T) {
+	db := newTestDB(t)
+	seedArtistRow(t, db, "ar1", "Radiohead", "", true)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+
+	if err := SyncAll(ctx, db, &stubResolver{}, &stubDiscography{}, &stubAlbums{}, 24*time.Hour); err == nil {
+		t.Fatal("SyncAll() expected context error, got nil")
+	}
+}
+
+// TestDiscographySyncer_AdapterForwards verifies the adapter produced by
+// NewDiscographySyncer forwards to the real SyncArtistDiscography so that the
+// App's wiring uses the actual MusicBrainz client.
+func TestDiscographySyncer_AdapterForwards(t *testing.T) {
+	db := newTestDB(t)
+	artistID := "nav-adapter"
+	artistMBID := "bbbbbbbb-cccc-dddd-eeee-ffffffffffff"
+	seedArtistRow(t, db, artistID, "Adapter Artist", "", true)
+
+	server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/xml")
+		resp := mbReleaseGroupListResponse(
+			mbReleaseGroupXML("rg1", "Adapter Album", "Album", "", artistMBID, "Adapter Artist", "2020-01-01"),
+			1,
+		)
+		w.Write([]byte(resp))
+	})
+	defer server.Close()
+
+	syncer := NewDiscographySyncer(newTestClient(server.URL))
+	releases, err := syncer.SyncArtistDiscography(context.Background(), db, artistID, artistMBID, 24*time.Hour)
+	if err != nil {
+		t.Fatalf("adapter SyncArtistDiscography() error = %v", err)
+	}
+	if len(releases) != 1 {
+		t.Fatalf("adapter expected 1 release, got %d", len(releases))
+	}
+	if releases[0].RGID != "rg1" {
+		t.Errorf("adapter release RGID = %q, want rg1", releases[0].RGID)
+	}
+}

internal/navidrome/client.go

@@ -47,6 +47,19 @@ func NewClient(cfg config.NavidromeConfig) (*NavidromeClient, error) {
 	return &NavidromeClient{client: client}, nil
 }
 
+// NewClientUnauthenticated builds a NavidromeClient without contacting the
+// server. It is intended for dependency injection in tests (where the
+// navidromeClientFactory seam in main is overridden) and for callers that want
+// to defer or skip authentication. Production wiring should prefer NewClient.
+func NewClientUnauthenticated(cfg config.NavidromeConfig) *NavidromeClient {
+	return &NavidromeClient{client: &subsonic.Client{
+		Client:     &http.Client{Timeout: 30 * time.Second},
+		BaseUrl:    cfg.URL,
+		User:       cfg.User,
+		ClientName: "naviwatcher",
+	}}
+}
+
 // Ping checks connectivity to the Navidrome server.
 // Returns nil if the server is reachable and responds with a valid Subsonic OK status.
 func (nc *NavidromeClient) Ping() error {