From 7204a48c1cab3a28105841975dcb847509a51192 Mon Sep 17 00:00:00 2001 From: Christophe Vila Date: Mon, 27 Jul 2026 06:44:39 +0200 Subject: [PATCH] refactor(sync): remove dead Backfill/IncrementalSync exported wrappers Both were only reachable via the periodic background sync loop removed in 4d2cbe4 -- nothing in production calls them anymore, only tests did. FullSync already calls backfillCore/incrementalSyncCore directly. Rewrite the affected tests to call the *Core functions (still exported within the package) instead, dropping the now-redundant standalone SyncRun-recording assertion covered by TestFullSync_RecordsOneCombinedSyncRun. --- backend/internal/store/isolation_test.go | 2 +- backend/internal/store/syncruns.go | 10 ++- backend/internal/store/users_test.go | 2 +- backend/internal/sync/service.go | 64 +++++-------------- backend/internal/sync/service_test.go | 78 +++++++++++------------- 5 files changed, 57 insertions(+), 99 deletions(-) diff --git a/backend/internal/store/isolation_test.go b/backend/internal/store/isolation_test.go index cf6ac74..69aa6c1 100644 --- a/backend/internal/store/isolation_test.go +++ b/backend/internal/store/isolation_test.go @@ -137,7 +137,7 @@ func TestIsolation_SyncStateAndRunsNeverLeakAcrossUsers(t *testing.T) { t.Fatalf("userA's UpdateSyncState leaked into userB's sync_state: %+v", stateB) } - runID, err := db.StartSyncRun(ctx, userA, SyncKindBackfill) + runID, err := db.StartSyncRun(ctx, userA, SyncKindFull) if err != nil { t.Fatalf("StartSyncRun(a): %v", err) } diff --git a/backend/internal/store/syncruns.go b/backend/internal/store/syncruns.go index ad04c36..65b6954 100644 --- a/backend/internal/store/syncruns.go +++ b/backend/internal/store/syncruns.go @@ -7,12 +7,10 @@ import ( ) const ( - SyncKindBackfill = "backfill" - SyncKindIncremental = "incremental" - // SyncKindFull is a manually-triggered "Sync now" pass: Backfill followed - // by IncrementalSync followed by FillPendingDetails, recorded as one run - // so the reported activity count covers the whole action instead of only - // whichever stage happened to finish last. + // SyncKindFull is a manually-triggered "Sync now" pass: backfillCore + // followed by incrementalSyncCore followed by FillPendingDetails, + // recorded as one run so the reported activity count covers the whole + // action instead of only whichever stage happened to finish last. SyncKindFull = "full" SyncStatusRunning = "running" diff --git a/backend/internal/store/users_test.go b/backend/internal/store/users_test.go index 93bff87..02020d6 100644 --- a/backend/internal/store/users_test.go +++ b/backend/internal/store/users_test.go @@ -126,7 +126,7 @@ func TestDeleteUser_RemovesUserAndCascadesEverything(t *testing.T) { if err := db.UpdateSyncState(ctx, userID, "2020-01-01", true); err != nil { t.Fatalf("UpdateSyncState: %v", err) } - if _, err := db.StartSyncRun(ctx, userID, SyncKindBackfill); err != nil { + if _, err := db.StartSyncRun(ctx, userID, SyncKindFull); err != nil { t.Fatalf("StartSyncRun: %v", err) } diff --git a/backend/internal/sync/service.go b/backend/internal/sync/service.go index 080c713..7be0f41 100644 --- a/backend/internal/sync/service.go +++ b/backend/internal/sync/service.go @@ -96,7 +96,7 @@ func (s *Service) setProgress(done, total int) { s.progressMu.Unlock() } -// Backfill pages backward in Config.BackfillWindowDays windows until +// backfillCore pages backward in Config.BackfillWindowDays windows until // Profile.BackfillHorizonDays is reached or Garmin returns an empty page. // The horizon is read fresh from the profile on every call (not fixed at // server startup), so a user-edited value takes effect on the very next @@ -106,27 +106,10 @@ func (s *Service) setProgress(done, total int) { // completed backfill, or is a fast no-op if the configured horizon is // already fully covered -- it does not re-walk years of already-known // history. Widening the horizon between calls resumes further back instead -// of re-fetching everything. -func (s *Service) Backfill(ctx context.Context) error { - runID, err := s.db.StartSyncRun(ctx, s.userID, store.SyncKindBackfill) - if err != nil { - return err - } - - total, err := s.backfillCore(ctx) - if err != nil { - msg := err.Error() - s.db.FinishSyncRun(ctx, s.userID, runID, total, &msg) - return err - } - return s.db.FinishSyncRun(ctx, s.userID, runID, total, nil) -} - -// backfillCore holds Backfill's actual fetch logic, without the SyncRun -// bookkeeping, so FullSync can run it as one step of a single combined run -// instead of its own separately-recorded one. The returned count reflects -// whatever was fetched even when an error is also returned, matching -// Backfill's own partial-progress-on-error behavior. +// of re-fetching everything. Used by FullSync as one step of its single +// combined SyncRun; there is no standalone entrypoint for this anymore +// (the periodic background sync loop that used to call one is gone -- see +// 4d2cbe4 refactor: remove automatic background incremental sync). func (s *Service) backfillCore(ctx context.Context) (int, error) { profile, err := s.db.GetProfile(ctx, s.userID) if err != nil { @@ -191,25 +174,10 @@ func (s *Service) backfillCore(ctx context.Context) (int, error) { return total, nil } -// IncrementalSync fetches activities from just before the latest known -// activity (or a short recent window if none exist yet) through today. -func (s *Service) IncrementalSync(ctx context.Context) error { - runID, err := s.db.StartSyncRun(ctx, s.userID, store.SyncKindIncremental) - if err != nil { - return err - } - - n, err := s.incrementalSyncCore(ctx) - if err != nil { - msg := err.Error() - s.db.FinishSyncRun(ctx, s.userID, runID, n, &msg) - return err - } - return s.db.FinishSyncRun(ctx, s.userID, runID, n, nil) -} - -// incrementalSyncCore holds IncrementalSync's actual fetch logic, without -// the SyncRun bookkeeping -- see backfillCore. +// incrementalSyncCore fetches activities from just before the latest known +// activity (or a short recent window if none exist yet) through today. Used +// by FullSync as one step of its single combined SyncRun -- see +// backfillCore's comment for why there's no standalone entrypoint. func (s *Service) incrementalSyncCore(ctx context.Context) (int, error) { start := s.now().AddDate(0, 0, -s.cfg.IncrementalOverlapDays) if latest, ok, err := s.db.LatestActivityStartTime(ctx, s.userID); err == nil && ok { @@ -221,15 +189,11 @@ func (s *Service) incrementalSyncCore(ctx context.Context) (int, error) { return newCount, err } -// FullSync performs a complete manual "Sync now" pass -- Backfill (resumes -// from the watermark), then IncrementalSync (catches anything new since the -// latest known activity), then FillPendingDetails -- recorded as a single -// SyncRun. Backfill and IncrementalSync each record their own SyncRun when -// called on their own (used by the periodic background loop), but a manual -// sync runs both back to back, and FillPendingDetails records no run at all; -// showing the user only the most recently *recorded* run (IncrementalSync's) -// would silently hide however many activities Backfill fetched. Recording -// one combined run makes the reported count match the whole action. +// FullSync performs a complete manual "Sync now" pass -- backfillCore +// (resumes from the watermark), then incrementalSyncCore (catches anything +// new since the latest known activity), then FillPendingDetails -- +// recorded as a single SyncRun so the reported activity count covers the +// whole action instead of only whichever stage happened to finish last. func (s *Service) FullSync(ctx context.Context, detailFillLimit int) error { runID, err := s.db.StartSyncRun(ctx, s.userID, store.SyncKindFull) if err != nil { diff --git a/backend/internal/sync/service_test.go b/backend/internal/sync/service_test.go index 3b93bae..f85d064 100644 --- a/backend/internal/sync/service_test.go +++ b/backend/internal/sync/service_test.go @@ -52,7 +52,7 @@ func setBackfillHorizon(t *testing.T, db *store.DB, userID int64, days int) { } } -func TestBackfill_StoresActivitiesAndRecordsSyncRun(t *testing.T) { +func TestBackfillCore_StoresActivities(t *testing.T) { db := openTestDB(t) ctx := context.Background() userID := provisionTestUser(t, db) @@ -63,8 +63,12 @@ func TestBackfill_StoresActivitiesAndRecordsSyncRun(t *testing.T) { }} svc := NewService(m, db, userID, Config{}, fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("Backfill: %v", err) + total, err := svc.backfillCore(ctx) + if err != nil { + t.Fatalf("backfillCore: %v", err) + } + if total != 1 { + t.Errorf("backfillCore returned total = %d, want 1", total) } activities, err := db.ListActivities(ctx, userID, store.ActivityFilter{}) @@ -77,14 +81,6 @@ func TestBackfill_StoresActivitiesAndRecordsSyncRun(t *testing.T) { if activities[0].GarminActivityID != 1 { t.Errorf("GarminActivityID = %d, want 1", activities[0].GarminActivityID) } - - runs, err := db.ListSyncRuns(ctx, userID, 10) - if err != nil { - t.Fatalf("ListSyncRuns: %v", err) - } - if len(runs) != 1 || runs[0].Status != store.SyncStatusSuccess { - t.Fatalf("expected 1 successful sync run, got %+v", runs) - } } func TestAlignWorkoutTargets_MatchesLapsToFlattenedSteps(t *testing.T) { @@ -195,7 +191,7 @@ func TestTargetHRRange_CustomRangeAndZoneNumberViaKarvonen(t *testing.T) { } } -func TestBackfill_SkipsNonRunningActivities(t *testing.T) { +func TestBackfillCore_SkipsNonRunningActivities(t *testing.T) { db := openTestDB(t) ctx := context.Background() userID := provisionTestUser(t, db) @@ -212,8 +208,8 @@ func TestBackfill_SkipsNonRunningActivities(t *testing.T) { }} svc := NewService(m, db, userID, Config{}, fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("Backfill: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("backfillCore: %v", err) } activities, err := db.ListActivities(ctx, userID, store.ActivityFilter{}) @@ -258,8 +254,8 @@ func TestFillPendingDetailsAndClassify_EndToEnd(t *testing.T) { } svc := NewService(m, db, userID, Config{MinConfidence: 0.5}, fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("Backfill: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("backfillCore: %v", err) } // A workout kind that should cleanly match the seeded activity's pace. @@ -328,8 +324,8 @@ func TestFillPendingDetails_ResolvesWorkoutTargetsOntoLaps(t *testing.T) { } svc := NewService(m, db, userID, Config{}, fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("Backfill: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("backfillCore: %v", err) } if err := svc.FillPendingDetails(ctx, 10); err != nil { t.Fatalf("FillPendingDetails: %v", err) @@ -385,8 +381,8 @@ func TestFillPendingDetails_OneExtraTrailingLapKeepsOtherLapsTargets(t *testing. } svc := NewService(m, db, userID, Config{}, fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("Backfill: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("backfillCore: %v", err) } if err := svc.FillPendingDetails(ctx, 10); err != nil { t.Fatalf("FillPendingDetails: %v", err) @@ -454,7 +450,7 @@ func TestBuildMetricContext_DerivesIsRace(t *testing.T) { } } -func TestBackfill_SecondRunIsANoOpOnceHorizonFullyCovered(t *testing.T) { +func TestBackfillCore_SecondRunIsANoOpOnceHorizonFullyCovered(t *testing.T) { db := openTestDB(t) ctx := context.Background() userID := provisionTestUser(t, db) @@ -466,8 +462,8 @@ func TestBackfill_SecondRunIsANoOpOnceHorizonFullyCovered(t *testing.T) { fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) setBackfillHorizon(t, db, userID, 10) - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("first Backfill: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("first backfillCore: %v", err) } firstCallCount := m.GetActivitiesCalls if firstCallCount == 0 { @@ -482,11 +478,11 @@ func TestBackfill_SecondRunIsANoOpOnceHorizonFullyCovered(t *testing.T) { t.Fatalf("expected backfill_complete=true after covering the full horizon, got %+v", state) } - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("second Backfill: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("second backfillCore: %v", err) } if m.GetActivitiesCalls != firstCallCount { - t.Errorf("second Backfill made %d more GetActivities call(s); want 0 (should be a no-op once horizon is covered)", + t.Errorf("second backfillCore made %d more GetActivities call(s); want 0 (should be a no-op once horizon is covered)", m.GetActivitiesCalls-firstCallCount) } } @@ -503,8 +499,8 @@ func TestResetAll_AllowsFreshBackfillAfterwards(t *testing.T) { fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) setBackfillHorizon(t, db, userID, 10) - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("first Backfill: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("first backfillCore: %v", err) } firstCallCount := m.GetActivitiesCalls @@ -519,8 +515,8 @@ func TestResetAll_AllowsFreshBackfillAfterwards(t *testing.T) { t.Fatalf("expected 0 activities after ResetAll, got %d", len(activities)) } - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("Backfill after reset: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("backfillCore after reset: %v", err) } if m.GetActivitiesCalls <= firstCallCount { t.Errorf("expected Backfill after ResetAll to call GetActivities again (fresh pull), call count stayed at %d", m.GetActivitiesCalls) @@ -534,7 +530,7 @@ func TestResetAll_AllowsFreshBackfillAfterwards(t *testing.T) { } } -func TestBackfill_ResumesFromWatermarkWhenHorizonGrows(t *testing.T) { +func TestBackfillCore_ResumesFromWatermarkWhenHorizonGrows(t *testing.T) { db := openTestDB(t) ctx := context.Background() userID := provisionTestUser(t, db) @@ -546,8 +542,8 @@ func TestBackfill_ResumesFromWatermarkWhenHorizonGrows(t *testing.T) { svc := NewService(m, db, userID, Config{BackfillWindowDays: 10}, now) setBackfillHorizon(t, db, userID, 10) - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("first Backfill: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("first backfillCore: %v", err) } firstCallCount := m.GetActivitiesCalls @@ -556,8 +552,8 @@ func TestBackfill_ResumesFromWatermarkWhenHorizonGrows(t *testing.T) { // make progress toward the new, deeper horizon. setBackfillHorizon(t, db, userID, 30) svc2 := NewService(m, db, userID, Config{BackfillWindowDays: 10}, now) - if err := svc2.Backfill(ctx); err != nil { - t.Fatalf("second Backfill: %v", err) + if _, err := svc2.backfillCore(ctx); err != nil { + t.Fatalf("second backfillCore: %v", err) } if m.GetActivitiesCalls <= firstCallCount { t.Errorf("expected additional GetActivities calls when horizon grows, got %d total (was %d)", m.GetActivitiesCalls, firstCallCount) @@ -655,8 +651,8 @@ func TestFillPendingDetails_ReportsLiveProgress(t *testing.T) { svc := NewService(m, db, userID, Config{InterCallDelay: 150 * time.Millisecond}, fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) - if err := svc.Backfill(ctx); err != nil { - t.Fatalf("Backfill: %v", err) + if _, err := svc.backfillCore(ctx); err != nil { + t.Fatalf("backfillCore: %v", err) } if p := svc.Progress(); p.Total != 0 { @@ -704,11 +700,11 @@ func TestService_TwoUsersSyncIndependently(t *testing.T) { svcA := NewService(mA, db, userA, Config{}, fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) svcB := NewService(mB, db, userB, Config{}, fixedNow(time.Date(2026, 7, 11, 0, 0, 0, 0, time.UTC))) - if err := svcA.Backfill(ctx); err != nil { - t.Fatalf("Backfill(a): %v", err) + if _, err := svcA.backfillCore(ctx); err != nil { + t.Fatalf("backfillCore(a): %v", err) } - if err := svcB.Backfill(ctx); err != nil { - t.Fatalf("Backfill(b): %v", err) + if _, err := svcB.backfillCore(ctx); err != nil { + t.Fatalf("backfillCore(b): %v", err) } activitiesA, err := db.ListActivities(ctx, userA, store.ActivityFilter{})