Port upstream PR #358: sync error propagation and per-provider sync metrics (gap fill)

[joestump-agent]

Ports joestump#358, implementing joestump#326.

This is a gap-fill port, not a wholesale cherry-pick: the fork's parallel audit-remediation work already implemented most of upstream joestump#358's intent. A blind cherry-pick conflicts in cmd/server/main.go, internal/services/sync.go, and internal/services/sync_error_test.go.

Gap analysis

Upstream joestump#358 change	Fork already had	This PR adds
Sync() joins history + playlist errors via errors.Join	Yes (sync.go Sync() already computed errors.Join(histErr, plErr))	Wraps the joined error as sync completed with errors: %w and logs full sync completed with errors instead of unconditionally logging success
syncHistory/syncPlaylists aggregate per-provider failures instead of swallowing via continue	No — both functions unconditionally returned nil (return allAdded, nil), so the errors.Join in Sync() never saw a failure and Sync() always returned nil on provider errors	Per-provider errs slices, wrapped as <provider>: history/playlist sync failed: %w, returned via errors.Join. Backoff skips remain throttling (no error), matching upstream
SyncProvider() surfaces aggregated errors (makes handler "Sync Failed" toasts reachable)	No — logged phase errors, always returned nil	Returns provider sync completed with errors: %w; the Sync Failed branches in preferences.go, spotify_auth.go, lastfm_auth.go, auth.go, recent.go are now reachable
True per-provider metric.sync (listens_synced, playlists_synced, duration_ms, success, error)	Yes (providerSyncStats map + per-provider emission)	Nothing structural; kept the fork's design (see review notes on the upstream pre-seeding defect)
Background tick loops: atomic error counters + per-tick WaitGroup join before metric.background_tick	Yes — and better: fork's shared runPerUserTick in cmd/server/main.go has atomic.Int64 errCount, per-tick sync.WaitGroup, plus an in-flight guard and users_skipped accounting upstream lacks	Nothing — cmd/server/main.go untouched
sync_error_test.go coverage	Partial — assertions still said "Sync swallows per-provider errors"	Flipped 4 stale assertions; added the issue joestump#326 acceptance tests (TestSyncer_AllProvidersFail_SyncReturnsErrorAndMetricSyncFails, TestSyncer_PartialFailure_SurfacesErrorButPreservesPartialSuccess with JSON log capture of metric.sync) plus a fork-specific TestSyncer_SyncProvider_SurfacesProviderError

Fork-specific behavior preserved: backoff/circuit-breaker flow (including ClearProviderBackoff), SPEC-0015 email notifier hooks, cross-provider listen dedup, playlist reconciliation (reconcileInactivePlaylists), and the metric convention that backoff-skipped providers emit no metric.sync event.

Defects found in upstream joestump#358 (reviewed during port)

1. Backoff-skipped providers report success=true: upstream pre-seeds an outcome for every active provider, so a provider skipped by backoff (never ran) emits metric.sync with success=true, duration_ms=0 — a failing-and-throttled provider looks healthy. Kept the fork's approach: providers that never ran emit no event. Not ported.
2. Listens persisted before a mid-sync failure vanish from the total: upstream's history error path records totalAdded in the metric outcome but continues before allAdded += totalAdded, so batches persisted before the failure are dropped from the returned total (metric and total disagree). Fixed here: the error path adds totalAdded to both the per-provider stat and the aggregate.
3. Persistence failure still logs a "completed" sync event (handling here is forward-looking): upstream's syncPlaylists appends the persistPlaylists error but then falls through to log EventTypeSyncCompleted ("Completed syncing playlists...") for the failed provider. This PR restructures that branch so a persistPlaylists error logs EventTypeSyncFailed, marks the provider failed in metric.sync, and joins the aggregated error — but note the branch cannot currently fire: the fork's persistPlaylists (like persistListens) handles every internal DB failure with Warn + continue and unconditionally returns nil. The same applies to the history-side persistListens error path inside the GetRecentListens callback. Both branches are forward-looking and only activate once the persist layer aggregates and returns real errors; until then, DB failures mid-sync still log success=true. Code comments mark both branches as forward-looking. Making the persist layer return real errors is tracked in follow-up Persist layer swallows DB errors — sync reports success on mid-sync DB failure #35.

Follow-ups filed

• Persist layer swallows DB errors — sync reports success on mid-sync DB failure #35 — Persist layer swallows DB errors — sync reports success on mid-sync DB failure (activates the forward-looking branches above).
• Sync failure UX follow-ups from error propagation #36 — Sync failure UX follow-ups from error propagation (reset-flow partial-failure handling, connect-flow full-Sync toast attribution, backoff-skip vs "Sync Complete" toast).

Test results

make generate && go build ./... && go vet ./... && go test ./...

All green — 31 packages ok, 0 failures. New/updated tests in internal/services/sync_error_test.go all pass:

• TestSyncer_ProviderError_TriggersBackoffRetriable (flipped: first sync now errors)
• TestSyncer_ProviderError_FatalNotRetried (flipped)
• TestSyncer_ClearProviderBackoff_AllowsRetryAfterFatal (flipped first assertion)
• TestSyncer_HistoryCallbackError_DoesNotPersistPartialData (flipped)
• TestSyncer_AllProvidersFail_SyncReturnsErrorAndMetricSyncFails (new)
• TestSyncer_PartialFailure_SurfacesErrorButPreservesPartialSuccess (new)
• TestSyncer_SyncProvider_SurfacesProviderError (new)

References: joestump#358 · joestump#326

🤖 Generated with Claude Code

[joestump-agent]

Adversarial review — verdict: APPROVE (no blocking findings). (Submitted as COMMENT because GitHub blocks approving from the PR author's account.)

Review of the sync error propagation port (upstream joestump#358 / issue joestump#326). I traced every failure path in syncHistory/syncPlaylists, audited all 13 call sites of Sync()/SyncProvider()/SyncRecentListens()/SyncPlaylists() (handlers + cmd/server/main.go tick loop), and verified mechanically: make generate, go build ./..., go vet ./..., gofmt -l clean, full go test ./... green, and the new error tests stable under -race -count=5.

The core error flow is correct: errors.Join on an empty/all-nil slice returns nil, per-provider wrapping uses the correct loop-local provider name, backoff skips deliberately do not surface as errors (Sync returns nil, matching the fork's backoff design and the tests), partial-batch history totals are counted exactly once (failure path at sync.go:361/367 and success path at 410-415 are mutually exclusive), and the background tick loop (runPerUserTick, cmd/server/main.go:239-281) only logs and increments errCount — no loop abort, no scheduling skip, so the new non-nil returns actually fix the previously-always-zero metric.background_tick error count rather than regressing anything.

Non-blocking findings:

1. NON-BLOCKING — The new persistPlaylists failure branch is unreachable dead code; deviation (c) cannot fire. internal/services/sync.go:533-546 handles persistPlaylists returning an error, but persistPlaylists (sync.go:761-886) unconditionally returns a nil error — every internal failure (managed-playlist query at 778, existence check at 820-823, update at 839-842, create at 860-863) is Warn+continue, and the only return is return addedCount, updatedCount, nil at 885. Same for persistListens (always nil at sync.go:710), which makes the history batch-callback error return at sync.go:348-351 equally dead. Concrete consequence: if the DB fails mid-sync, every save Warns, the sync still returns nil, metric.sync logs success=true, and the handler shows "Sync Complete" — the issue joestump#326 silent-failure class still exists for the persistence layer. The branch is harmless and correct if persist ever grows an error return, but the PR's claimed deviation (c) ("persistPlaylists failure now logs EventTypeSyncFailed and marks metric.sync failed") describes behavior that cannot currently occur and cannot be tested. Suggest either making persistListens/persistPlaylists return real errors in a follow-up, or noting in the PR description that this branch is forward-looking only.

2. NON-BLOCKING — Reset flow now skips metadata enrichment on any partial sync failure. internal/handlers/preferences.go:1205-1231: Sync() error → publish "Reset Failed" → return, skipping MetadataSvc.SyncAll. Scenario: user has Spotify (healthy) + Navidrome (one retriable timeout); user resets all data; Spotify re-syncs thousands of listens, but the user sees "Reset Failed" and the freshly reset library gets zero enrichment. With metadata.enabled=false there is no background enrichment loop to recover, so data stays unenriched until a manual task. Previously (Sync always nil) enrichment always ran and the toast said "Reset Complete". Suggest running enrichment even when Sync returns a partial error, or a "Reset Partial" toast.

3. NON-BLOCKING — Provider-specific "Sync Failed" toasts can blame the wrong provider. internal/handlers/spotify_auth.go:286-296 and internal/handlers/lastfm_auth.go:247-257 call full Sync() (all active providers) after connecting one provider. Scenario: user connects Spotify successfully while Navidrome is down → aggregated error contains only the navidrome failure → toast reads "Sync Failed — Failed to sync Spotify data. Please try again later." Retrying won't help and the message blames the wrong provider. Consider SyncProvider(ctx, u, TypeSpotify) here, or provider-aware messaging.

4. NON-BLOCKING — Failed-then-"Complete" whiplash on immediate manual retry. First manual sync of a broken provider now correctly toasts "Sync Failed" (preferences.go:260-271 etc.). But RecordFailure sets NextRetryAt in the future even on the first retriable failure (resilience.go:257-259), so an immediate retry hits ShouldSkip (resilience.go:282-300), the provider is silently skipped, SyncProvider returns nil, and the handler toasts "Sync Complete — Navidrome sync complete" although nothing ran and the provider is still broken. Same misleading-success shape when the provider isn't active at all (sync.go:174-177 returns nil). This inconsistency is newly visible because of this PR's (correct) error surfacing; consider surfacing "skipped: backing off" to manual-sync callers in a follow-up.

Probed without success (i.e., verified clean): errors.Join nil-handling and wrapper semantics (%w around Join keeps errors.Is/As working); double-counting of partial-batch listens; per-provider name attribution in wrapped errors and metric.sync stats; within-run backoff interaction (a provider that fails history is backoff-skipped in the same run's playlist phase, so the aggregate correctly carries only the history error); metric.sync success/error field consistency on partial success (covered by TestSyncer_PartialFailure_SurfacesErrorButPreservesPartialSuccess); JSON log-capture test brittleness (Sync is fully synchronous on the test goroutine, JSONHandler is line-oriented, scanner buffer sufficient; stable under -race -count=5); and revert-detection (the new assert.Error/require.Error assertions fail immediately if the join is dropped).

---

Generated by Claude Code