Story #326: Propagate sync errors and fix sync metrics

[joestump]

What / Why

Part of epic #317. Sync failures were structurally invisible: syncHistory/syncPlaylists ended with return allAdded, nil and every failure path was continue, so Sync()'s error handling, the metric.sync success flag, and the "Sync Failed" notification at internal/handlers/spotify_auth.go:279 were all dead code. Separately, the background-tick error counters raced and always logged stale/zero values, and metric.background_tick/metric.sync durations measured spawn time and aggregate totals respectively.

Changes

• Error propagation (internal/services/sync.go) — syncHistory and syncPlaylists aggregate per-provider failures with errors.Join and return them. Sync() and SyncProvider() surface the aggregated error. Partial success is preserved: one provider failing does not stop the others, and successful providers' data is persisted before the error is returned. Backoff skips are deliberate throttling and are not treated as new failures (so the post-failure "skip" sync returns nil, matching existing backoff tests). Playlist persistence failures are also aggregated (defensive; persistPlaylists currently always returns nil error).
• Per-provider metric.sync (REQ-BG-003) — Sync() now tracks a providerSyncOutcome per active provider (listens, playlists, duration, errors across both phases) and emits one metric.sync per provider carrying that provider's own values and success flag, instead of one event per provider each carrying aggregate totals for all providers.
• Racy tick counters (cmd/server/main.go) — syncErrors/metadataErrors/plSyncErrors are now atomic.Int64, incremented from per-user goroutines, and each tick joins its own goroutines via a per-tick sync.WaitGroup before logging metric.background_tick. The shared shutdown WaitGroup usage is untouched (per-tick WaitGroup is additive; Fix graceful shutdown ordering and background-job cancellation #342's shutdown restructure is unaffected).
• metric.background_tick duration_ms — because the metric is emitted after the per-tick join, it now measures actual tick work instead of goroutine spawn time (REQ-BG-001/002). Side effect: ticks no longer overlap within a loop, matching the metadata loop's existing synchronous precedent (missed ticker fires are dropped by time.Ticker).

"Sync Failed" notification path (traced)

internal/handlers/spotify_auth.go:279 publishes the "Sync Failed" notification when h.Syncer.Sync() returns an error. That branch was unreachable; TestSyncer_AllProvidersFail_SyncReturnsErrorAndMetricSyncFails now proves Sync() returns an error when the provider fails immediately, making the notification path live. The equivalent previously-dead error branches in auth.go, lastfm_auth.go, preferences.go (manual sync tasks via SyncProvider/SyncRecentListens/SyncPlaylists), and recent.go are activated the same way.

Test evidence

• New acceptance test TestSyncer_AllProvidersFail_SyncReturnsErrorAndMetricSyncFails: every provider fails → Sync() returns an error naming each provider, and each provider's metric.sync (captured via slog.NewJSONHandler) logs success=false with a non-empty error.
• New test TestSyncer_PartialFailure_SurfacesErrorButPreservesPartialSuccess: failing Spotify + healthy Navidrome → error surfaces naming only Spotify, Navidrome's 2 listens persist, and metric.sync shows success=false/success=true with per-provider listens_synced (attribution fix).
• Updated 5 existing tests that asserted the old swallow behavior ("Sync should not surface per-provider errors") to assert errors now surface; backoff-skip second-sync assertions unchanged.
• go test ./internal/services/... -race — pass; make test (full suite) — green; go vet clean; gofmt -l clean; golangci-lint v1.64.8 (CI-pinned version, run via go run) — clean on changed packages. Local golangci-lint is v2.11.4 and cannot read the repo's v1 config, so the pinned version was used.

Governing artifacts

• ADR-0019 (structured metrics observability) — metric.sync / metric.background_tick schemas
• ADR-0020 (error handling and resilience) — aggregation, backoff interplay
• SPEC observability REQ-BG-001, REQ-BG-002, REQ-BG-003
• SPEC listen-playlist-sync REQ-SYNC-011 (history failure does not abort playlist sync; partial success)

No design-doc updates required (the spec already mandated the per-provider semantics this PR implements).

Closes #326

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Independent review — verdict: APPROVE (no blocking defects)

Verified adversarially in a detached worktree of the PR head (495dc41): regenerated ent/templ, go build ./..., go vet, gofmt -l clean, full go test ./... green, go test ./internal/services/... -race -count=1 green, and re-ran the 5 updated + 2 new tests individually. I traced the code paths myself rather than trusting the PR body; the traced claims hold.

What I verified holds

• Partial success preserved. One provider failing does not block others; the healthy provider's listens persist before the aggregated error is returned (internal/services/sync.go:146-153, proven end-to-end against a real ent DB by TestSyncer_PartialFailure_SurfacesErrorButPreservesPartialSuccess). Both new tests genuinely discriminate old vs. new behavior — the old aggregate-totals emission would fail the ListensSynced and per-provider success assertions.
• errors.Join × ClassifyError (Adopt HTTPStatusError across all API clients; fix fatal-error classification #354) is safe. Classification for backoff happens per-provider on the raw error before any joining (sync.go:365, sync.go:484), and the only other ClassifyError consumers of sync errors (internal/notifications/notifier.go:71, internal/notifications/templates.go:38) receive the raw per-provider error via NotifyIfNeeded. Nothing anywhere classifies Sync()'s wrapped/joined return, so no class-precedence ambiguity exists today.
• "Sync Failed" notification path is live. internal/handlers/spotify_auth.go:279 now receives a non-nil error on an immediately-failing first sync (fresh backoff key → first failure surfaces). Same for auth.go:129, lastfm_auth.go:240, and the preferences.go task handlers (866, 916, 1193), whose "Task Failed" toasts were equally dead before.
• Concurrency is sound. tickWG.Add(1) sits immediately before each spawn with no continue in between; defer tickWG.Done() runs on every exit path including the ctx.Done() semaphore bail-out; Load() only happens after Wait() (proper happens-before). Shutdown cannot hang on the tick join: per-user goroutines exit on cancellation, and the hard-exit timer at cmd/server/main.go:658 plus the timeout-bounded wg.Wait (main.go:679-686) cap the worst case — same budget as before this PR.
• No test weakening. All 5 updates flip NoError → Error (strictly stronger); backoff-skip second-sync assertions are unchanged and correct.
• No collision with PR Fix track-matching correctness (rune/byte) and per-tick performance (#330) #357 (Fix track-matching correctness (rune/byte) and per-tick performance #330): it touches internal/config/config.go, playlist_sync.go, track_matcher.go — no overlap with this PR's cmd/server/main.go / internal/services/sync.go.

Findings (all non-blocking)

MEDIUM — backoff-skip emits success=true with zeros, which inverts the observability signal for dead providers. internal/services/sync.go:108-113 pre-seeds an outcome for every active provider, so a provider skipped by backoff in both phases emits metric.sync success=true, listens_synced=0, duration_ms=0 every tick. After a fatal error (revoked credentials), the provider logs exactly one success=false event and then success=true forever — an ADR-0019-style jq success-rate query reads ~100% for a permanently broken provider. REQ-BG-003 ties emission to "after each per-user sync operation completes"; a backoff skip completes no operation, so the fabricated success event is not spec-required. Recommend either suppressing emission for providers skipped in all phases, or adding a skipped=true attribute (with a one-line spec amendment). Fine as a fast-follow, but I'd fix it — the author flagged it and the flag is warranted.

LOW — stale spec sequence diagram. docs/openspec/specs/listen-playlist-sync/spec.md:115 still says Syncer-->>Scheduler: done (error logged, not returned). That line now describes the pre-#326 swallow contract this PR removes. The PR body's "no design-doc updates required" is true for the normative REQs but not for this diagram — update it to "error logged and returned (aggregated)".

LOW — aggregate undercount on mid-fetch failure. When GetRecentListens fails after some batches persisted, the partial listens are (correctly) counted in metric.sync (sync.go:390 records totalAdded) but excluded from syncHistory's returned allAdded (sync.go:404, success path only), so the "full sync completed with errors" log's listens_synced (sync.go:150-151) can under-report vs. the per-provider metrics it sits next to. Pick one convention.

LOW — one failure now logs at Error 3-4 times per tick. sync.go:367 (fetch failure) → sync.go:118 ("failed to sync history") → sync.go:150 ("full sync completed with errors") → main.go:261 ("background sync failed"). For a retriable flap on a 5-minute tick that's noisy; consider demoting the intermediate two.

NOTE — tick-join cadence trade-off (and #342 overlap). The per-tick join means the next sync tick cannot start until the slowest user finishes; time.Ticker drops missed fires, so effective cadence degrades to max(interval, slowest-user duration). Within-tick parallelism is preserved (per-user goroutines, semaphore-bounded), so a huge first sync doesn't starve other users within a tick — it only delays everyone's next tick. Acceptable for a single-operator deployment (ADR-0019's own framing) and necessary for an honest REQ-BG-001 duration_ms. Related: the REQ-ENRICH-041 governing comment at main.go ("syncMetadataForUsers blocks synchronously") was false before this PR — the function spawned goroutines and returned — and only becomes true now. That means story #342's planned explicit in-flight guard for metadata ticks is largely satisfied by this join; #342 should be re-scoped so it doesn't add a redundant second guard on top.

NOTE — factory-stage failures remain invisible. getActiveProviders (sync.go:240-249) logs and skips a provider whose factory errors (per REQ-SYNC-003), so a provider broken at construction (e.g. auth-decrypt failure) emits no metric.sync, returns nil from Sync(), and never reaches the "Sync Failed" notification. Spec-compliant, but the #326 fix intentionally doesn't cover that failure class — worth remembering if "connected but never syncs, no error shown" ever gets reported.

🤖 Posted on behalf of @joestump by Claude.