diff --git a/backend/internal/api/api_test.go b/backend/internal/api/api_test.go index ed01148..ae9cea1 100644 --- a/backend/internal/api/api_test.go +++ b/backend/internal/api/api_test.go @@ -669,12 +669,20 @@ func TestSyncStatus_ReportsPhaseAwareProgressAndWorkoutsPending(t *testing.T) { ctx := newCtx() workoutID := int64(555) - if _, err := db.UpsertActivity(ctx, userID, store.Activity{ + activityID, err := db.UpsertActivity(ctx, userID, store.Activity{ GarminActivityID: 1, WorkoutID: &workoutID, StartTimeUTC: "2026-07-11 06:00:00", RawJSON: "{}", - }); err != nil { + }) + if err != nil { t.Fatalf("UpsertActivity: %v", err) } + // ActivitiesMissingWorkout only surfaces activities whose details have + // already been fetched (see store.ActivitiesMissingWorkout) -- an + // activity is only eligible for a workout fetch once fillActivityDetails + // has given it laps to align target pace/HR bands against. + if err := db.SetActivityDetails(ctx, userID, activityID, "{}"); err != nil { + t.Fatalf("SetActivityDetails: %v", err) + } rec := doJSON(t, s.Router(), http.MethodGet, "/api/sync/status", nil) if rec.Code != http.StatusOK { diff --git a/backend/internal/store/activities.go b/backend/internal/store/activities.go index d25f398..f5021fb 100644 --- a/backend/internal/store/activities.go +++ b/backend/internal/store/activities.go @@ -276,16 +276,24 @@ func (db *DB) CountActivitiesMissingDetails(ctx context.Context, userID int64) ( // ActivitiesMissingWorkout returns userID's activities that have a // structured workout (WorkoutID set at initial upsert time, straight from // Garmin's activity summary) but haven't had get_workout_by_id fetched yet. -// Independently queryable from ActivitiesMissingDetails: workout_id is -// known well before any detail fetch, and workout_raw_json is only ever -// set by SetActivityWorkout, so this also picks up an activity whose -// details were fetched successfully in some prior run but whose workout -// fetch failed back then -- ActivitiesMissingDetails would never surface -// that activity again (details_fetched_at/splits_fetched_at are already -// set), silently losing its target pace/HR bands forever without this. +// Gated on details_fetched_at IS NOT NULL: fillActivityWorkout resolves each +// lap's target pace/HR band via alignWorkoutTargets, which needs the +// activity's laps already written by fillActivityDetails -- without this +// gate, a structured-workout activity whose details/laps haven't been +// fetched yet would fall into a separate, independently LIMIT-bounded batch +// than fillPendingDetails's, get its workout_raw_json set against zero laps +// (a silent no-op alignment), and then never be retried once its laps +// finally arrive, since workout_raw_json IS NULL is the only signal this +// query has left. This is still independent of splits_fetched_at (unlike +// ActivitiesMissingDetails, which requires both): splits/laps aren't needed +// to resolve workout targets, only details_fetched_at is. It also still +// covers the intended retry case -- an activity whose workout fetch +// previously failed after details succeeded always has details_fetched_at +// already set, so it's still surfaced here -- while excluding activities +// that simply haven't been processed by fillActivityDetails at all yet. func (db *DB) ActivitiesMissingWorkout(ctx context.Context, userID int64, limit int) ([]Activity, error) { rows, err := db.QueryContext(ctx, `SELECT `+activityColumns+` FROM activities - WHERE user_id = ? AND workout_id IS NOT NULL AND workout_raw_json IS NULL + WHERE user_id = ? AND workout_id IS NOT NULL AND workout_raw_json IS NULL AND details_fetched_at IS NOT NULL ORDER BY start_time_utc DESC LIMIT ?`, userID, limit) if err != nil { return nil, fmt.Errorf("list activities missing workout for user %d: %w", userID, err) @@ -306,11 +314,12 @@ func (db *DB) ActivitiesMissingWorkout(ctx context.Context, userID int64, limit // CountActivitiesMissingWorkout returns how many of userID's activities // still need get_workout_by_id fetched, regardless of any per-call batch // limit -- used to report overall remaining work, mirroring -// CountActivitiesMissingDetails. +// CountActivitiesMissingDetails. See ActivitiesMissingWorkout for why this +// is additionally gated on details_fetched_at IS NOT NULL. func (db *DB) CountActivitiesMissingWorkout(ctx context.Context, userID int64) (int, error) { var n int err := db.QueryRowContext(ctx, `SELECT COUNT(*) FROM activities - WHERE user_id = ? AND workout_id IS NOT NULL AND workout_raw_json IS NULL`, userID).Scan(&n) + WHERE user_id = ? AND workout_id IS NOT NULL AND workout_raw_json IS NULL AND details_fetched_at IS NOT NULL`, userID).Scan(&n) if err != nil { return 0, fmt.Errorf("count activities missing workout for user %d: %w", userID, err) } diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index d494670..3b89c7a 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -356,7 +356,23 @@ func TestCurrentAssignment_ScopedToOwningUser(t *testing.T) { } } -func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T) { +// TestActivitiesMissingWorkout_OnlyIncludesActivitiesWithDetailsAlreadyFetched +// is a regression test for a bug where ActivitiesMissingWorkout selected +// activities purely on workout_id IS NOT NULL AND workout_raw_json IS NULL, +// with no regard for whether get_activity_details had ever run for that +// activity. Since fillPendingDetails and fillPendingWorkouts are each +// independently LIMIT-bounded over different candidate sets, a +// structured-workout activity could fall inside the workouts pass's window +// while still outside the details pass's window on a large first backfill. +// fillActivityWorkout would then call LapsForActivity on an activity with no +// laps yet written, compute alignWorkoutTargets against zero laps (a +// no-op), and unconditionally call SetActivityWorkout anyway -- permanently +// marking workout_raw_json non-NULL before the activity ever had a chance +// to get real target pace/HR bands once its laps finally arrived. The fix +// gates the query on details_fetched_at IS NOT NULL, so an activity is only +// eligible for a workout fetch once fillActivityDetails has actually given +// it laps to align against. +func TestActivitiesMissingWorkout_OnlyIncludesActivitiesWithDetailsAlreadyFetched(t *testing.T) { db := openTestDB(t) ctx := context.Background() userID, err := db.ProvisionUser(ctx, "test-sub", "Test") @@ -365,6 +381,9 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T) } workoutID := int64(999) + // Activity a: has a workout_id, but details/splits have never been + // fetched (the exact shape of the bug above). Must now be EXCLUDED -- + // this is the regression assertion for the bug. withWorkoutNoDetails := Activity{ GarminActivityID: 1, WorkoutID: &workoutID, StartTimeUTC: "2026-07-01 06:00:00", RawJSON: "{}", @@ -374,6 +393,11 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T) t.Fatalf("UpsertActivity (a): %v", err) } + // Activity b: has a workout_id, and its details/splits have already + // been fetched (e.g. its workout fetch failed in some prior run after + // details succeeded). This is the one case ActivitiesMissingWorkout must + // still surface, since ActivitiesMissingDetails would never pick this + // activity up again once details_fetched_at/splits_fetched_at are set. withWorkoutAndDetails := Activity{ GarminActivityID: 2, WorkoutID: &workoutID, StartTimeUTC: "2026-07-02 06:00:00", RawJSON: "{}", @@ -382,10 +406,6 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T) if err != nil { t.Fatalf("UpsertActivity (b): %v", err) } - // Simulate an activity whose details/splits were already fetched in a - // prior run, but whose workout fetch failed back then -- this is exactly - // the case ActivitiesMissingWorkout must still surface, since it's - // queried independently of details_fetched_at/splits_fetched_at. if err := db.SetActivityDetails(ctx, userID, idB, "{}"); err != nil { t.Fatalf("SetActivityDetails (b): %v", err) } @@ -393,6 +413,7 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T) t.Fatalf("SetActivitySplitsFetched (b): %v", err) } + // Activity c: no workout_id at all. Must be excluded. noWorkout := Activity{ GarminActivityID: 3, StartTimeUTC: "2026-07-03 06:00:00", RawJSON: "{}", } @@ -400,6 +421,7 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T) t.Fatalf("UpsertActivity (c): %v", err) } + // Activity d: already has its workout_raw_json set. Must be excluded. alreadyHasWorkout := Activity{ GarminActivityID: 4, WorkoutID: &workoutID, StartTimeUTC: "2026-07-04 06:00:00", RawJSON: "{}", @@ -416,23 +438,24 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T) if err != nil { t.Fatalf("CountActivitiesMissingWorkout: %v", err) } - if n != 2 { - t.Fatalf("CountActivitiesMissingWorkout = %d, want 2 (activities a and b)", n) + if n != 1 { + t.Fatalf("CountActivitiesMissingWorkout = %d, want 1 (activity b only -- activity a must be excluded since its details were never fetched)", n) } pending, err := db.ActivitiesMissingWorkout(ctx, userID, 10) if err != nil { t.Fatalf("ActivitiesMissingWorkout: %v", err) } - if len(pending) != 2 { - t.Fatalf("ActivitiesMissingWorkout returned %d activities, want 2", len(pending)) + if len(pending) != 1 { + t.Fatalf("ActivitiesMissingWorkout returned %d activities, want 1", len(pending)) + } + if pending[0].ID != idB { + t.Errorf("ActivitiesMissingWorkout = %+v, want only activity b (id %d)", pending, idB) } - ids := map[int64]bool{} for _, a := range pending { - ids[a.ID] = true - } - if !ids[idA] || !ids[idB] { - t.Errorf("ActivitiesMissingWorkout = %+v, want to include activities a and b", pending) + if a.ID == idA { + t.Errorf("ActivitiesMissingWorkout incorrectly included activity a (workout_id set but details never fetched) -- regression for the silent-loss bug") + } } }