From 67f4ec27ab97d5ac7e2ad527e93b3e27daeb3edb Mon Sep 17 00:00:00 2001 From: Christophe Vila Date: Mon, 27 Jul 2026 18:00:38 +0200 Subject: [PATCH] docs: add design spec for permanently-missing-workout handling A workout deleted on Garmin's side after being linked to an activity 404s forever on get_workout_by_id, and today's fillPendingWorkouts retries it every sync indefinitely with no user-visible feedback -- just a perpetual "1 more workout pending" nudge. Distinguishes a definitive 404 (via garminconnect's own GarminConnectNotFoundError) from a transient failure, and stops retrying the former. Co-Authored-By: Claude Sonnet 5 --- .../2026-07-27-workout-not-found-design.md | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-27-workout-not-found-design.md diff --git a/docs/superpowers/specs/2026-07-27-workout-not-found-design.md b/docs/superpowers/specs/2026-07-27-workout-not-found-design.md new file mode 100644 index 0000000..e74ad8f --- /dev/null +++ b/docs/superpowers/specs/2026-07-27-workout-not-found-design.md @@ -0,0 +1,108 @@ +# Stop retrying a permanently-missing Garmin workout forever — Design + +**Status:** Approved, ready for implementation planning. +**Origin:** user report -- `/api/sync/status`'s `workouts_pending` count stuck at 1 forever, no +matter how many times "Sync now" is clicked. + +## Root cause + +`fillPendingWorkouts`/`fillActivityWorkout` (`backend/internal/sync/service.go:339-371`) treats +every `get_workout_by_id` failure identically: log it, leave `workout_raw_json` `NULL`, and let +`ActivitiesMissingWorkout` naturally retry it on the next sync. This is the right behavior for a +*transient* failure (network blip, rate limiting), but wrong for a genuine HTTP 404 -- the +activity's `workout_id` points at a workout that was deleted on Garmin's side after the activity +was recorded (confirmed via the user's own backend log: a 404 for a specific `workout_id`). A 404 +is definitive and will never succeed on retry, yet the current code retries it every single sync, +forever, with the failure only ever logged server-side (`log.Printf`) -- never surfaced to the +user beyond a perpetual, unexplained "1 more workout pending" nudge. + +## Fix + +An activity can legitimately end up with `workout_id` set but `workout_raw_json` permanently +`NULL` when the workout can't be found on Garmin -- `workout_raw_json` stays `NULL` forever (no +fabricated data), but a new, separate marker records "confirmed missing" distinctly from "not yet +fetched," so `ActivitiesMissingWorkout`/`CountActivitiesMissingWorkout` stop counting it. + +### `internal/garmin/pyscript/wrapper.py` + +`garminconnect` already defines `GarminConnectNotFoundError` (a subclass of +`GarminConnectConnectionError`) specifically for this case -- its own docstring says "so callers +can now catch a missing resource specifically (e.g. deleting an already-deleted workout)." It's +raised by `connectapi()` (used by `get_workout_by_id` and other lookups) whenever the underlying +HTTP response is a 404. + +`dispatch()`'s generic exception handler (used by every `cmd`, including the generic `call` +dispatcher that `get_workout_by_id` goes through) gains a check: if the caught exception is a +`GarminConnectNotFoundError`, the returned JSON error response gets an additional +`"not_found": true` field alongside the existing `"error"` string. This is deliberately generic +(any Garmin API call that 404s gets this marker, not just workouts) -- the decision about what to +*do* with a not-found error stays method-specific in the Go/sync layer. + +### `internal/garmin/client.go` + +- `wireResponse` gains `NotFound bool `json:"not_found,omitempty"``. +- A new sentinel: `var ErrNotFound = errors.New("garmin: resource not found")`. +- `roundTrip`: when `resp.Error != "" && resp.NotFound`, the returned error wraps `ErrNotFound` + (`fmt.Errorf("%s: %s: %w", cmdName, resp.Error, ErrNotFound)`), so callers can + `errors.Is(err, garmin.ErrNotFound)` regardless of which method was called. + +### `internal/store` (schema + queries) + +- `schema.sql`: `activities` gains `workout_not_found_at TEXT` (nullable), placed next to + `workout_raw_json`, following the exact same style as the existing `details_fetched_at`/ + `splits_fetched_at` timestamp markers. +- New method `SetActivityWorkoutNotFound(ctx, userID, activityID) error`, mirroring + `SetActivitySplitsFetched`'s exact shape (`UPDATE activities SET workout_not_found_at = + datetime('now'), updated_at = datetime('now') WHERE id = ? AND user_id = ?`). +- `ActivitiesMissingWorkout`/`CountActivitiesMissingWorkout` both gain + `AND workout_not_found_at IS NULL` in their `WHERE` clause. +- `UpsertActivity`'s `ON CONFLICT DO UPDATE` already never touches `details_fetched_at`/ + `details_raw_json`/`splits_fetched_at`/`workout_raw_json` -- `workout_not_found_at` follows the + same pattern, so it's untouched by a later re-sync and the "confirmed missing" marker persists + indefinitely once set. +- `docs/DATABASE.md` regenerated via `go run ./cmd/dumpschema` after the schema edit. + +### `internal/sync/service.go` + +In `fillPendingWorkouts`'s loop, when `fillActivityWorkout` returns an error: +- If `errors.Is(err, garmin.ErrNotFound)`: call `db.SetActivityWorkoutNotFound(ctx, s.userID, + a.ID)` and log at Info level ("workout not found on Garmin, likely deleted -- marking as such, + will not retry"). No further retry. +- Otherwise: keep the existing behavior exactly as it is today (log a warning, leave + `workout_raw_json` `NULL`, naturally retried on the next sync). + +### `internal/garmin/mock` + +`mock.Client`'s existing `WorkoutErrByID map[int64]error` field (added in a previous plan) needs +no shape change -- a test simulating a 404 just sets the mapped error to +`fmt.Errorf("...: %w", garmin.ErrNotFound)` (or `garmin.ErrNotFound` directly), which +`errors.Is` picks up the same way a real wrapped error would. + +## Non-goals + +- No change to `fillActivityDetails`/`fillPendingActivityDetails`'s error handling (a + details/splits fetch failure still aborts the batch and surfaces as a sync error, as it does + today) -- this fix is scoped to workouts specifically, matching the confirmed root cause. If a + details/splits 404 ever turns out to need the same treatment, that's a separate, unconfirmed + problem to investigate on its own evidence. +- No frontend change -- `workouts_pending` simply stops counting a confirmed-missing workout, so + the existing SyncModal/banner UI needs nothing new. +- No handling for the (very unlikely) edge case of Garmin re-linking a *different* `workout_id` + to the same activity after `workout_not_found_at` was set for an earlier one -- accepted as an + edge case not worth the added complexity. + +## Testing + +- `internal/garmin/pyscript/tests/test_wrapper.py`: a new test mirroring the existing + `test_call_propagates_garminconnect_exception_as_error`, but raising + `GarminConnectNotFoundError` and asserting the response includes `"not_found": true`. +- `internal/garmin` (Go): a test asserting `roundTrip` wraps the error with `ErrNotFound` when + the wire response sets `not_found: true`. +- `internal/store`: extend the existing `ActivitiesMissingWorkout` test (or add a new one) with a + fixture that has `workout_not_found_at` set, asserting it's excluded from both + `ActivitiesMissingWorkout` and `CountActivitiesMissingWorkout`. +- `internal/sync`: a test using `mock.Client.WorkoutErrByID` set to a `garmin.ErrNotFound`-wrapped + error, asserting `fillPendingWorkouts`/`FillPendingDetails` completes without error, the + activity's `workout_not_found_at` gets set, and it's excluded from a subsequent + `CountActivitiesMissingWorkout` call (i.e., confirming it does NOT get retried on a second call, + unlike today's forever-retry behavior for this exact scenario).