fix(store): gate ActivitiesMissingWorkout on details already fetched
fillPendingDetails and fillPendingWorkouts are each independently LIMIT-bounded over different candidate sets, so on a large first backfill a structured-workout activity could fall inside the workouts pass's window while still outside the details pass's window. fillActivityWorkout would then align workout targets against zero laps (details/laps never written yet) and unconditionally mark workout_raw_json non-NULL, permanently losing that activity's target pace/HR bands once its real laps arrived later -- silently, with no error. Add details_fetched_at IS NOT NULL to ActivitiesMissingWorkout's and CountActivitiesMissingWorkout's WHERE clause: an activity is only eligible for a workout fetch once its laps actually exist to align against. This still covers the intended retry case (an activity whose workout fetch previously failed always has details_fetched_at already set) while excluding never-yet-processed activities. Rename/rewrite the store test to assert the corrected exclusion (count=1, not 2) as an explicit regression test, and fix the TestSyncStatus_ReportsPhaseAwareProgressAndWorkoutsPending API fixture, which exercised the same buggy shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -669,12 +669,20 @@ func TestSyncStatus_ReportsPhaseAwareProgressAndWorkoutsPending(t *testing.T) {
|
|||||||
ctx := newCtx()
|
ctx := newCtx()
|
||||||
|
|
||||||
workoutID := int64(555)
|
workoutID := int64(555)
|
||||||
if _, err := db.UpsertActivity(ctx, userID, store.Activity{
|
activityID, err := db.UpsertActivity(ctx, userID, store.Activity{
|
||||||
GarminActivityID: 1, WorkoutID: &workoutID,
|
GarminActivityID: 1, WorkoutID: &workoutID,
|
||||||
StartTimeUTC: "2026-07-11 06:00:00", RawJSON: "{}",
|
StartTimeUTC: "2026-07-11 06:00:00", RawJSON: "{}",
|
||||||
}); err != nil {
|
})
|
||||||
|
if err != nil {
|
||||||
t.Fatalf("UpsertActivity: %v", err)
|
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)
|
rec := doJSON(t, s.Router(), http.MethodGet, "/api/sync/status", nil)
|
||||||
if rec.Code != http.StatusOK {
|
if rec.Code != http.StatusOK {
|
||||||
|
|||||||
@@ -276,16 +276,24 @@ func (db *DB) CountActivitiesMissingDetails(ctx context.Context, userID int64) (
|
|||||||
// ActivitiesMissingWorkout returns userID's activities that have a
|
// ActivitiesMissingWorkout returns userID's activities that have a
|
||||||
// structured workout (WorkoutID set at initial upsert time, straight from
|
// structured workout (WorkoutID set at initial upsert time, straight from
|
||||||
// Garmin's activity summary) but haven't had get_workout_by_id fetched yet.
|
// Garmin's activity summary) but haven't had get_workout_by_id fetched yet.
|
||||||
// Independently queryable from ActivitiesMissingDetails: workout_id is
|
// Gated on details_fetched_at IS NOT NULL: fillActivityWorkout resolves each
|
||||||
// known well before any detail fetch, and workout_raw_json is only ever
|
// lap's target pace/HR band via alignWorkoutTargets, which needs the
|
||||||
// set by SetActivityWorkout, so this also picks up an activity whose
|
// activity's laps already written by fillActivityDetails -- without this
|
||||||
// details were fetched successfully in some prior run but whose workout
|
// gate, a structured-workout activity whose details/laps haven't been
|
||||||
// fetch failed back then -- ActivitiesMissingDetails would never surface
|
// fetched yet would fall into a separate, independently LIMIT-bounded batch
|
||||||
// that activity again (details_fetched_at/splits_fetched_at are already
|
// than fillPendingDetails's, get its workout_raw_json set against zero laps
|
||||||
// set), silently losing its target pace/HR bands forever without this.
|
// (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) {
|
func (db *DB) ActivitiesMissingWorkout(ctx context.Context, userID int64, limit int) ([]Activity, error) {
|
||||||
rows, err := db.QueryContext(ctx, `SELECT `+activityColumns+` FROM activities
|
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)
|
ORDER BY start_time_utc DESC LIMIT ?`, userID, limit)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf("list activities missing workout for user %d: %w", userID, err)
|
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
|
// CountActivitiesMissingWorkout returns how many of userID's activities
|
||||||
// still need get_workout_by_id fetched, regardless of any per-call batch
|
// still need get_workout_by_id fetched, regardless of any per-call batch
|
||||||
// limit -- used to report overall remaining work, mirroring
|
// 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) {
|
func (db *DB) CountActivitiesMissingWorkout(ctx context.Context, userID int64) (int, error) {
|
||||||
var n int
|
var n int
|
||||||
err := db.QueryRowContext(ctx, `SELECT COUNT(*) FROM activities
|
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 {
|
if err != nil {
|
||||||
return 0, fmt.Errorf("count activities missing workout for user %d: %w", userID, err)
|
return 0, fmt.Errorf("count activities missing workout for user %d: %w", userID, err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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)
|
db := openTestDB(t)
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
userID, err := db.ProvisionUser(ctx, "test-sub", "Test")
|
userID, err := db.ProvisionUser(ctx, "test-sub", "Test")
|
||||||
@@ -365,6 +381,9 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T)
|
|||||||
}
|
}
|
||||||
|
|
||||||
workoutID := int64(999)
|
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{
|
withWorkoutNoDetails := Activity{
|
||||||
GarminActivityID: 1, WorkoutID: &workoutID,
|
GarminActivityID: 1, WorkoutID: &workoutID,
|
||||||
StartTimeUTC: "2026-07-01 06:00:00", RawJSON: "{}",
|
StartTimeUTC: "2026-07-01 06:00:00", RawJSON: "{}",
|
||||||
@@ -374,6 +393,11 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T)
|
|||||||
t.Fatalf("UpsertActivity (a): %v", err)
|
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{
|
withWorkoutAndDetails := Activity{
|
||||||
GarminActivityID: 2, WorkoutID: &workoutID,
|
GarminActivityID: 2, WorkoutID: &workoutID,
|
||||||
StartTimeUTC: "2026-07-02 06:00:00", RawJSON: "{}",
|
StartTimeUTC: "2026-07-02 06:00:00", RawJSON: "{}",
|
||||||
@@ -382,10 +406,6 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T)
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("UpsertActivity (b): %v", err)
|
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 {
|
if err := db.SetActivityDetails(ctx, userID, idB, "{}"); err != nil {
|
||||||
t.Fatalf("SetActivityDetails (b): %v", err)
|
t.Fatalf("SetActivityDetails (b): %v", err)
|
||||||
}
|
}
|
||||||
@@ -393,6 +413,7 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T)
|
|||||||
t.Fatalf("SetActivitySplitsFetched (b): %v", err)
|
t.Fatalf("SetActivitySplitsFetched (b): %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Activity c: no workout_id at all. Must be excluded.
|
||||||
noWorkout := Activity{
|
noWorkout := Activity{
|
||||||
GarminActivityID: 3, StartTimeUTC: "2026-07-03 06:00:00", RawJSON: "{}",
|
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)
|
t.Fatalf("UpsertActivity (c): %v", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Activity d: already has its workout_raw_json set. Must be excluded.
|
||||||
alreadyHasWorkout := Activity{
|
alreadyHasWorkout := Activity{
|
||||||
GarminActivityID: 4, WorkoutID: &workoutID,
|
GarminActivityID: 4, WorkoutID: &workoutID,
|
||||||
StartTimeUTC: "2026-07-04 06:00:00", RawJSON: "{}",
|
StartTimeUTC: "2026-07-04 06:00:00", RawJSON: "{}",
|
||||||
@@ -416,23 +438,24 @@ func TestActivitiesMissingWorkout_IndependentOfDetailsFetchStatus(t *testing.T)
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("CountActivitiesMissingWorkout: %v", err)
|
t.Fatalf("CountActivitiesMissingWorkout: %v", err)
|
||||||
}
|
}
|
||||||
if n != 2 {
|
if n != 1 {
|
||||||
t.Fatalf("CountActivitiesMissingWorkout = %d, want 2 (activities a and b)", n)
|
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)
|
pending, err := db.ActivitiesMissingWorkout(ctx, userID, 10)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("ActivitiesMissingWorkout: %v", err)
|
t.Fatalf("ActivitiesMissingWorkout: %v", err)
|
||||||
}
|
}
|
||||||
if len(pending) != 2 {
|
if len(pending) != 1 {
|
||||||
t.Fatalf("ActivitiesMissingWorkout returned %d activities, want 2", len(pending))
|
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 {
|
for _, a := range pending {
|
||||||
ids[a.ID] = true
|
if a.ID == idA {
|
||||||
|
t.Errorf("ActivitiesMissingWorkout incorrectly included activity a (workout_id set but details never fetched) -- regression for the silent-loss bug")
|
||||||
}
|
}
|
||||||
if !ids[idA] || !ids[idB] {
|
|
||||||
t.Errorf("ActivitiesMissingWorkout = %+v, want to include activities a and b", pending)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user