Files
geniusrun/docs/superpowers/specs/2026-07-27-workout-not-found-design.md

109 lines
6.5 KiB
Markdown
Raw Normal View History

# 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).