From 6127c5c2d1cf19325f3efa73929c44d5756e5851 Mon Sep 17 00:00:00 2001 From: orchestrator-agent <20+orchestrator-agent@code.cubecraftcreations.com> Date: Sun, 11 Oct 2026 21:44:55 -0400 Subject: [PATCH] Photo backfill for failed dinners without a photo (Maestro 36c40130, Feature 23 task 1) (#205) Co-authored-by: orchestrator-agent <20+orchestrator-agent@code.cubecraftcreations.com> --- docs/ARCHITECTURE.md | 17 ++ internal/db/dinners.sql.go | 164 +++++++++++++ internal/db/querier.go | 20 ++ internal/dinners/backfill.go | 188 ++++++++++++++ internal/dinners/backfill_test.go | 378 +++++++++++++++++++++++++++++ internal/dinners/dinners.go | 14 +- internal/dinners/export_test.go | 15 ++ internal/dinners/grounding_test.go | 3 +- internal/server/dinners.go | 14 ++ internal/server/grounding_test.go | 40 +++ internal/server/server.go | 1 + migrations/queries/dinners.sql | 62 +++++ 12 files changed, 913 insertions(+), 3 deletions(-) create mode 100644 internal/dinners/backfill.go create mode 100644 internal/dinners/backfill_test.go diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0472353..9ab4786 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -259,6 +259,23 @@ automatic** (D-021). unarchived dinner still `pending` with no `ground_dinner` job for its revision queued or running is set `failed` with `interrupted`, so no dinner stays pending with nothing left to ground it. + Photo backfill. `POST /api/v1/settings/photo-backfill` (CSRF-checked) + enqueues a `backfill_dinner_photos` job and answers 202 `{id}` without + waiting for it; while one is queued or running it enqueues nothing and + answers with that job's id, so two passes never overlap, across restarts + too. The kind is refused by `POST /api/v1/jobs`. The job + grounds, one at a time with `Ground` on its name, every unarchived dinner + that is `failed` with no photo and no pasted `recipe_url` — grounded, + `manual` and `pending` dinners are never listed, and a pasted link is + never replaced by a search. Success sets grounding_status `grounded` as + the `ground_dinner` job does (the recipe keeps its source URL and site); + failure leaves the dinner `failed` with the new `grounding_error`, for + the fix-recipe flow. One dinner's failure never stops the others; a + dinner renamed, archived or grounded by hand meanwhile is left as it is. + Its result is `{dinners: [{dinner_id, outcome, reason}]}`, outcome + `grounded`, `failed` (with its reason) or `skipped`. A dinner whose write + fails is `failed` with `grounding_error` and left as it was; the others + still run, then the job fails with `internal_error`, keeping that result. `GET /api/v1/dinners/{id}/recipe-candidates` searches for the dinner's name and returns `{candidates: [{candidate_id, title, site_name, photo_url, minutes}]}`, at most 3, ranked as `Ground` diff --git a/internal/db/dinners.sql.go b/internal/db/dinners.sql.go index 986c0aa..be82f27 100644 --- a/internal/db/dinners.sql.go +++ b/internal/db/dinners.sql.go @@ -214,6 +214,26 @@ func (q *Queries) FailPendingDinnerGrounding(ctx context.Context, arg FailPendin return result.RowsAffected() } +const getActivePhotoBackfillJob = `-- name: GetActivePhotoBackfillJob :one +SELECT id +FROM jobs +WHERE kind = ?1 + AND status IN ('queued', 'running') + AND json_extract(payload, '$.dinner_id') IS NULL +ORDER BY id +LIMIT 1 +` + +// The id of the full photo backfill job (no dinner_id) of kind queued or +// running. StartPhotoBackfill enqueues one only when there is none, so +// there is at most one. Returns sql.ErrNoRows when there is none. +func (q *Queries) GetActivePhotoBackfillJob(ctx context.Context, kind string) (string, error) { + row := q.db.QueryRowContext(ctx, getActivePhotoBackfillJob, kind) + var id string + err := row.Scan(&id) + return id, err +} + const getDinner = `-- name: GetDinner :one SELECT id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision FROM dinners WHERE id = ? @@ -344,6 +364,58 @@ func (q *Queries) GroundDinner(ctx context.Context, arg GroundDinnerParams) (Din return i, err } +const groundFailedDinner = `-- name: GroundFailedDinner :execrows +UPDATE dinners +SET recipe_id = ?1, + photo_id = ?2, + method = COALESCE(method, ?3), + minutes_total = COALESCE(minutes_total, ?4), + difficulty = COALESCE(difficulty, ?5), + grounding_status = 'grounded', + grounding_error = NULL, + updated_at = ?6 +WHERE id = ?7 + AND status <> 'archived' + AND grounding_status = 'failed' + AND photo_id IS NULL + AND recipe_url IS NULL + AND grounding_revision = ?8 +` + +type GroundFailedDinnerParams struct { + RecipeID sql.NullString + PhotoID sql.NullString + Method sql.NullString + MinutesTotal sql.NullInt64 + Difficulty sql.NullString + UpdatedAt string + ID string + GroundingRevision int64 +} + +// Records a photo backfill's grounding of a dinner it listed, while the +// dinner is still unarchived, failed, photoless, without a recipe_url and at +// the grounding_revision it was listed at; a dinner grounded by hand, +// renamed or archived meanwhile is left as it is (0 rows). Like +// GroundPendingDinner it keeps a method, minutes_total or difficulty a +// member already set. +func (q *Queries) GroundFailedDinner(ctx context.Context, arg GroundFailedDinnerParams) (int64, error) { + result, err := q.db.ExecContext(ctx, groundFailedDinner, + arg.RecipeID, + arg.PhotoID, + arg.Method, + arg.MinutesTotal, + arg.Difficulty, + arg.UpdatedAt, + arg.ID, + arg.GroundingRevision, + ) + if err != nil { + return 0, err + } + return result.RowsAffected() +} + const groundPendingDinner = `-- name: GroundPendingDinner :execrows UPDATE dinners SET recipe_id = ?1, @@ -444,6 +516,64 @@ func (q *Queries) ListDinnersByStatus(ctx context.Context, status string) ([]Din return items, nil } +const listPhotoBackfillDinners = `-- name: ListPhotoBackfillDinners :many +SELECT id, name, normalized_name, status, origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, photo_id, grounding_status, updated_by, created_at, updated_at, recipe_url, grounding_error, grounding_revision FROM dinners +WHERE status <> 'archived' + AND grounding_status = 'failed' + AND photo_id IS NULL + AND recipe_url IS NULL + AND id = COALESCE(?1, id) +ORDER BY normalized_name, id +` + +// The dinners a photo backfill grounds by name: unarchived, failed, with no +// photo and no pasted recipe_url (the household's own link is never +// replaced by a search), ordered by normalized name. A non-NULL id narrows +// the list to that dinner. Grounded, manual and pending dinners are never +// listed: they have a recipe, or a ground_dinner job grounding them. +func (q *Queries) ListPhotoBackfillDinners(ctx context.Context, id sql.NullString) ([]Dinner, error) { + rows, err := q.db.QueryContext(ctx, listPhotoBackfillDinners, id) + if err != nil { + return nil, err + } + defer rows.Close() + var items []Dinner + for rows.Next() { + var i Dinner + if err := rows.Scan( + &i.ID, + &i.Name, + &i.NormalizedName, + &i.Status, + &i.Origin, + &i.Method, + &i.MinutesTotal, + &i.Difficulty, + &i.Cuisine, + &i.IsFavorite, + &i.RecipeID, + &i.PhotoID, + &i.GroundingStatus, + &i.UpdatedBy, + &i.CreatedAt, + &i.UpdatedAt, + &i.RecipeUrl, + &i.GroundingError, + &i.GroundingRevision, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listUnarchivedDinners = `-- name: ListUnarchivedDinners :many SELECT dinners.id, dinners.name, dinners.normalized_name, dinners.status, dinners.origin, dinners.method, dinners.minutes_total, dinners.difficulty, dinners.cuisine, dinners.is_favorite, dinners.recipe_id, dinners.photo_id, dinners.grounding_status, dinners.updated_by, dinners.created_at, dinners.updated_at, dinners.recipe_url, dinners.grounding_error, dinners.grounding_revision, recipes.source_url AS recipe_source_url FROM dinners @@ -560,6 +690,40 @@ func (q *Queries) ListUnplannedQuestLogDinners(ctx context.Context, fromWeekStar return items, nil } +const refailDinnerGrounding = `-- name: RefailDinnerGrounding :execrows +UPDATE dinners +SET grounding_error = ?1, + updated_at = ?2 +WHERE id = ?3 + AND status <> 'archived' + AND grounding_status = 'failed' + AND photo_id IS NULL + AND recipe_url IS NULL + AND grounding_revision = ?4 +` + +type RefailDinnerGroundingParams struct { + GroundingError sql.NullString + UpdatedAt string + ID string + GroundingRevision int64 +} + +// Records why a photo backfill could not ground a dinner it listed, under +// the same conditions as GroundFailedDinner (0 rows otherwise). +func (q *Queries) RefailDinnerGrounding(ctx context.Context, arg RefailDinnerGroundingParams) (int64, error) { + result, err := q.db.ExecContext(ctx, refailDinnerGrounding, + arg.GroundingError, + arg.UpdatedAt, + arg.ID, + arg.GroundingRevision, + ) + if err != nil { + return 0, err + } + return result.RowsAffected() +} + const setDinnerFavorite = `-- name: SetDinnerFavorite :one UPDATE dinners SET is_favorite = ?, diff --git a/internal/db/querier.go b/internal/db/querier.go index d1623b1..e19b54e 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -117,6 +117,10 @@ type Querier interface { // when there is none, so there is at most one. Returns sql.ErrNoRows when // there is none. GetActiveDraftJob(ctx context.Context, arg GetActiveDraftJobParams) (GetActiveDraftJobRow, error) + // The id of the full photo backfill job (no dinner_id) of kind queued or + // running. StartPhotoBackfill enqueues one only when there is none, so + // there is at most one. Returns sql.ErrNoRows when there is none. + GetActivePhotoBackfillJob(ctx context.Context, kind string) (string, error) // The batch still pending or running, oldest first. Returns sql.ErrNoRows // when there is none. GetActiveSuggestionBatch(ctx context.Context) (SuggestionBatch, error) @@ -175,6 +179,13 @@ type Querier interface { // unknown id, returns sql.ErrNoRows. A method, minutes_total or difficulty // no longer as observed is kept: a member edited it while it was grounding. GroundDinner(ctx context.Context, arg GroundDinnerParams) (Dinner, error) + // Records a photo backfill's grounding of a dinner it listed, while the + // dinner is still unarchived, failed, photoless, without a recipe_url and at + // the grounding_revision it was listed at; a dinner grounded by hand, + // renamed or archived meanwhile is left as it is (0 rows). Like + // GroundPendingDinner it keeps a method, minutes_total or difficulty a + // member already set. + GroundFailedDinner(ctx context.Context, arg GroundFailedDinnerParams) (int64, error) // Records the background grounding of an unarchived dinner still pending at // the grounding_revision it was grounded for; a dinner grounded some other // way, renamed or archived meanwhile is left as it is (0 rows). A method, @@ -206,6 +217,12 @@ type Querier interface { // The pending Rotation checks whose dinner is still tried, oldest plan // date first, with their dinners. ListPendingRotationReviews(ctx context.Context) ([]ListPendingRotationReviewsRow, error) + // The dinners a photo backfill grounds by name: unarchived, failed, with no + // photo and no pasted recipe_url (the household's own link is never + // replaced by a search), ordered by normalized name. A non-NULL id narrows + // the list to that dinner. Grounded, manual and pending dinners are never + // listed: they have a recipe, or a ground_dinner job grounding them. + ListPhotoBackfillDinners(ctx context.Context, id sql.NullString) ([]Dinner, error) // The days of plan plan_id with their dinners, Monday first. A day with // no dinner is left out. ListPlanDays(ctx context.Context, planID string) ([]ListPlanDaysRow, error) @@ -307,6 +324,9 @@ type Querier interface { // Records a new draft over plan id: its status and note, raising its // revision. RedraftWeekPlan(ctx context.Context, arg RedraftWeekPlanParams) error + // Records why a photo backfill could not ground a dinner it listed, under + // the same conditions as GroundFailedDinner (0 rows otherwise). + RefailDinnerGrounding(ctx context.Context, arg RefailDinnerGroundingParams) (int64, error) // Records a generation of list id from plan plan_id. RegenerateShoppingList(ctx context.Context, arg RegenerateShoppingListParams) error // Records a failed delivery and when to try again. The lease must still be diff --git a/internal/dinners/backfill.go b/internal/dinners/backfill.go new file mode 100644 index 0000000..0c06c00 --- /dev/null +++ b/internal/dinners/backfill.go @@ -0,0 +1,188 @@ +package dinners + +import ( + "context" + "database/sql" + "encoding/json" + "errors" + "fmt" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" +) + +// KindBackfillPhotos is the job that grounds, by name, the dinners left +// without a photo: those whose grounding failed and that have no pasted +// recipe_url (ListPhotoBackfillDinners). +const KindBackfillPhotos = "backfill_dinner_photos" + +// Photo backfill outcomes for one dinner. +const ( + BackfillGrounded = "grounded" + BackfillFailed = "failed" + // BackfillSkipped: the dinner was grounded by hand, renamed or archived + // while the backfill grounded it, so the backfill left it as it is. + BackfillSkipped = "skipped" +) + +// BackfillJob is the KindBackfillPhotos payload. DinnerID narrows the +// backfill to that dinner; empty backfills every dinner without a photo. +type BackfillJob struct { + DinnerID string `json:"dinner_id,omitempty"` +} + +// BackfillOutcome is what a photo backfill did for one dinner. Reason is +// set only for BackfillFailed: the Reason recorded as its grounding_error. +type BackfillOutcome struct { + DinnerID string `json:"dinner_id"` + Outcome string `json:"outcome"` + Reason *string `json:"reason"` +} + +// backfillResult is the KindBackfillPhotos result: the outcome of each +// dinner the backfill tried, in the order it tried them. +type backfillResult struct { + Dinners []BackfillOutcome `json:"dinners"` +} + +func (r backfillResult) PublicJSON() ([]byte, error) { return json.Marshal(r) } + +// StartPhotoBackfill enqueues a KindBackfillPhotos job over every dinner +// without a photo and returns its id, without waiting for it. While a full +// backfill is queued or running it enqueues nothing and returns that job's +// id instead, so two passes never ground the same dinner at once; the jobs +// table holds the claim, so it holds across restarts too. +func (s *Service) StartPhotoBackfill(ctx context.Context) (string, error) { + var id string + created := false + err := s.st.WithTx(ctx, func(tx *sql.Tx) error { + active, err := db.New(tx).GetActivePhotoBackfillJob(ctx, KindBackfillPhotos) + switch { + case errors.Is(err, sql.ErrNoRows): + case err != nil: + return fmt.Errorf("dinners: read active photo backfill: %w", err) + default: + id = active + return nil + } + id, err = s.backfillJob.EnqueueTx(ctx, tx, BackfillJob{}) + if err != nil { + return fmt.Errorf("dinners: enqueue photo backfill: %w", err) + } + created = true + return nil + }) + if err != nil { + return "", err + } + if created { + s.backfillJob.Wake() + } + return id, nil +} + +// errBackfillNotRecorded fails a photo backfill that could not record the +// outcome of one or more of its dinners. +var errBackfillNotRecorded = errors.New("dinners: photo backfill: dinner outcomes not recorded") + +// runBackfillJob grounds each dinner ListPhotoBackfillDinners lists for p, +// one at a time, with recipes.Grounder.Ground on its name: the first ranked +// candidate whose photo caches and whose ingredients normalize grounds it, +// with the recipe's source URL and site kept as any grounding keeps them. +// Success records the recipe and photo with grounding_status grounded; +// failure leaves the dinner failed with the new Reason as its +// grounding_error, for the household to fix its recipe by hand. One +// dinner's failure, its search's, model's or database write's, never stops +// the others. A failure to list the dinners, or the job's context ending, +// fails the job at once. A write that failed fails the job once every +// dinner has been tried, as a grounding job's failed write does, with the +// outcomes published as its result, so the job never reports done for a +// dinner whose outcome is not stored. +func (s *Service) runBackfillJob(ctx context.Context, p BackfillJob, publish func(backfillResult) error) (backfillResult, error) { + q := db.New(s.st.Reader) + rows, err := q.ListPhotoBackfillDinners(ctx, sql.NullString{String: p.DinnerID, Valid: p.DinnerID != ""}) + if err != nil { + return backfillResult{}, fmt.Errorf("dinners: photo backfill: list: %w", err) + } + res := backfillResult{Dinners: make([]BackfillOutcome, 0, len(rows))} + grounded, unrecorded := false, false + for _, row := range rows { + out, recorded, err := s.backfillDinner(ctx, row) + if err != nil { + return backfillResult{}, err + } + grounded = grounded || out.Outcome == BackfillGrounded + unrecorded = unrecorded || !recorded + res.Dinners = append(res.Dinners, out) + } + if grounded { + s.wake() + } + if unrecorded { + if err := publish(res); err != nil { + return backfillResult{}, errors.Join(errBackfillNotRecorded, err) + } + return backfillResult{}, errBackfillNotRecorded + } + return res, nil +} + +// backfillDinner grounds row, a dinner ListPhotoBackfillDinners listed, as +// runBackfillJob describes, and reports whether its outcome was recorded. +// Its error is only the job's context ending. +func (s *Service) backfillDinner(ctx context.Context, row db.Dinner) (BackfillOutcome, bool, error) { + out := BackfillOutcome{DinnerID: row.ID} + w := db.New(s.st.Writer) + gr, err := s.grounder.Ground(ctx, row.Name, row.Name) + if ctxErr := ctx.Err(); ctxErr != nil { + return BackfillOutcome{}, false, ctxErr + } + if err != nil { + reason := reasonFor(err, false) + // Only the reason is logged: the error may quote a dependency's + // response, which jobs never store, return or log. + s.logger.InfoContext(ctx, "dinner photo backfill failed", "dinner_id", row.ID, "reason", reason) + n, err := w.RefailDinnerGrounding(ctx, db.RefailDinnerGroundingParams{ + GroundingError: sql.NullString{String: reason, Valid: true}, + UpdatedAt: store.Now().String(), + ID: row.ID, + GroundingRevision: row.GroundingRevision, + }) + out, recorded := s.backfillRecorded(ctx, out, BackfillFailed, reason, n, err) + return out, recorded, nil + } + n, err := w.GroundFailedDinner(ctx, db.GroundFailedDinnerParams{ + RecipeID: sql.NullString{String: gr.RecipeID, Valid: true}, + PhotoID: sql.NullString{String: gr.PhotoID, Valid: true}, + Method: sql.NullString{String: gr.Method, Valid: true}, + MinutesTotal: nullInt(gr.MinutesTotal), + Difficulty: sql.NullString{String: gr.Difficulty, Valid: true}, + UpdatedAt: store.Now().String(), + ID: row.ID, + GroundingRevision: row.GroundingRevision, + }) + out, recorded := s.backfillRecorded(ctx, out, BackfillGrounded, "", n, err) + return out, recorded, nil +} + +// backfillRecorded is out once the backfill recorded outcome (with reason, +// for BackfillFailed) on the dinner, writing n rows with err: a dinner that +// changed meanwhile (no row) is BackfillSkipped, and one whose write failed +// is BackfillFailed with ReasonGroundingError, the dinner left as it was, +// and not recorded. +func (s *Service) backfillRecorded(ctx context.Context, out BackfillOutcome, outcome, reason string, n int64, err error) (BackfillOutcome, bool) { + switch { + case err != nil: + // The error is a database error, never a dependency's text. + s.logger.ErrorContext(ctx, "dinner photo backfill not recorded", "dinner_id", out.DinnerID, "error", err) + out.Outcome, reason = BackfillFailed, ReasonGroundingError + case n == 0: + out.Outcome, reason = BackfillSkipped, "" + default: + out.Outcome = outcome + } + if reason != "" { + out.Reason = &reason + } + return out, err == nil +} diff --git a/internal/dinners/backfill_test.go b/internal/dinners/backfill_test.go new file mode 100644 index 0000000..6d22243 --- /dev/null +++ b/internal/dinners/backfill_test.go @@ -0,0 +1,378 @@ +package dinners_test + +import ( + "database/sql" + "encoding/json" + "errors" + "slices" + "sync" + "testing" + "time" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/harness" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/dinners" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/jobs" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes/recipestest" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/store" +) + +// failedDinner stores a Regular named name whose grounding failed with +// no_recipe_found, without a photo or recipe_url, and returns its id. +func (e *env) failedDinner(t *testing.T, name string) string { + t.Helper() + q := db.New(e.st.Writer) + now := store.Now().String() + row, err := q.CreateDinner(t.Context(), db.CreateDinnerParams{ + ID: store.NewID(), + Name: name, + NormalizedName: harness.Normalize(name), + Status: dinners.StatusRegular, + Origin: "regular", + GroundingStatus: dinners.GroundingPending, + UpdatedBy: sql.NullString{String: member, Valid: true}, + CreatedAt: now, + UpdatedAt: now, + }) + if err != nil { + t.Fatal(err) + } + if _, err := q.FailPendingDinnerGrounding(t.Context(), db.FailPendingDinnerGroundingParams{ + GroundingError: sql.NullString{String: dinners.ReasonNoRecipe, Valid: true}, + UpdatedAt: now, + ID: row.ID, + GroundingRevision: row.GroundingRevision, + }); err != nil { + t.Fatal(err) + } + return row.ID +} + +func outcome(id, out string, reason *string) dinners.BackfillOutcome { + return dinners.BackfillOutcome{DinnerID: id, Outcome: out, Reason: reason} +} + +func checkOutcomes(t *testing.T, got, want []dinners.BackfillOutcome) { + t.Helper() + gj, _ := json.Marshal(got) + wj, _ := json.Marshal(want) + if string(gj) != string(wj) { + t.Fatalf("outcomes = %s, want %s", gj, wj) + } +} + +func TestBackfillGroundsFirstCandidateWithAPhoto(t *testing.T) { + t.Parallel() + e := newUnstartedEnv(t) + // curryA ranks first but its photo is gone; curryB's photo caches. + delete(e.images.JPEGs, "https://a.example/curry.jpg") + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 25)) + id := e.failedDinner(t, "Chicken curry") + + got, err := e.svc.RunBackfillJob(t.Context(), dinners.BackfillJob{}) + if err != nil { + t.Fatal(err) + } + checkOutcomes(t, got, []dinners.BackfillOutcome{outcome(id, dinners.BackfillGrounded, nil)}) + + row := e.dinner(t, id) + if row.GroundingStatus != dinners.GroundingGrounded || !row.PhotoID.Valid || !row.RecipeID.Valid || row.GroundingError.Valid { + t.Fatalf("dinner = %+v, want grounded with a photo and no error", row) + } + rec, err := db.New(e.st.Reader).GetRecipe(t.Context(), row.RecipeID.String) + if err != nil { + t.Fatal(err) + } + if rec.SourceUrl != curryB || rec.SiteName.String != "B Kitchen" || rec.PhotoID != row.PhotoID { + t.Errorf("recipe = %s from %q photo %s, want %s from B Kitchen photo %s", rec.SourceUrl, rec.SiteName.String, rec.PhotoID.String, curryB, row.PhotoID.String) + } + if got := e.search.Queries(); !slices.Equal(got, []string{"Chicken curry"}) { + t.Errorf("searched %q, want the dinner's name", got) + } +} + +func TestBackfillReusesCachedRecipeAndPhoto(t *testing.T) { + t.Parallel() + e := newUnstartedEnv(t) + e.search.Results["Curry chicken"] = []recipes.Candidate{{URL: curryA}} + // One reply: the second dinner must reuse the recipe the first stored. + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + first := e.failedDinner(t, "Chicken curry") + second := e.failedDinner(t, "Curry chicken") + + got, err := e.svc.RunBackfillJob(t.Context(), dinners.BackfillJob{}) + if err != nil { + t.Fatal(err) + } + checkOutcomes(t, got, []dinners.BackfillOutcome{ + outcome(first, dinners.BackfillGrounded, nil), + outcome(second, dinners.BackfillGrounded, nil), + }) + a, b := e.dinner(t, first), e.dinner(t, second) + if a.RecipeID != b.RecipeID || a.PhotoID != b.PhotoID || !a.PhotoID.Valid { + t.Errorf("dinners grounded in recipe %s photo %s and recipe %s photo %s, want the same cached ones", + a.RecipeID.String, a.PhotoID.String, b.RecipeID.String, b.PhotoID.String) + } + if n := len(e.model.Requests()); n != 1 { + t.Errorf("normalize_recipe ran %d times, want 1", n) + } + if n := slices.Index(e.pages.Fetched(), curryA); n < 0 || slices.Contains(e.pages.Fetched()[n+1:], curryA) { + t.Errorf("fetched %q, want %s fetched once", e.pages.Fetched(), curryA) + } +} + +func TestBackfillIsolatesEachDinnersFailure(t *testing.T) { + t.Parallel() + e := newUnstartedEnv(t) + e.search.Fail = map[string]error{"Broken search": errors.New("search provider down")} + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + // Listed by normalized name: both failures come before the curry. + broken := e.failedDinner(t, "Broken search") + curry := e.failedDinner(t, "Chicken curry") + absent := e.failedDinner(t, "Abalone surprise") + + got, err := e.svc.RunBackfillJob(t.Context(), dinners.BackfillJob{}) + if err != nil { + t.Fatal(err) + } + checkOutcomes(t, got, []dinners.BackfillOutcome{ + outcome(absent, dinners.BackfillFailed, ptr(dinners.ReasonNoRecipe)), + outcome(broken, dinners.BackfillFailed, ptr(dinners.ReasonSearchFailed)), + outcome(curry, dinners.BackfillGrounded, nil), + }) + for id, reason := range map[string]string{absent: dinners.ReasonNoRecipe, broken: dinners.ReasonSearchFailed} { + row := e.dinner(t, id) + if row.GroundingStatus != dinners.GroundingFailed || row.GroundingError.String != reason || row.PhotoID.Valid { + t.Errorf("dinner %s = %+v, want failed with %s and no photo", row.Name, row, reason) + } + } + if row := e.dinner(t, curry); row.GroundingStatus != dinners.GroundingGrounded || !row.PhotoID.Valid { + t.Errorf("curry = %+v, want grounded with a photo", row) + } +} + +func TestBackfillSkipsDinnersNotMissingAPhoto(t *testing.T) { + t.Parallel() + e := newEnv(t) + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + e.waitJob(t, *c.GroundingJobID) + if row := e.dinner(t, c.ID); row.GroundingStatus != dinners.GroundingGrounded { + t.Fatalf("dinner = %+v, want grounded", row) + } + // A failed dinner with a pasted link keeps that link for a fix by hand. + linked, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Mystery pie"), RecipeURL: ptr("https://d.example/pie")}, member) + if err != nil { + t.Fatal(err) + } + e.waitJob(t, *linked.GroundingJobID) + if row := e.dinner(t, linked.ID); row.GroundingStatus != dinners.GroundingFailed { + t.Fatalf("linked dinner = %+v, want failed", row) + } + archived := e.failedDinner(t, "Old stew") + if _, err := e.svc.Archive(t.Context(), archived, member); err != nil { + t.Fatal(err) + } + searched := len(e.search.Queries()) + + id, err := e.svc.StartPhotoBackfill(t.Context()) + if err != nil { + t.Fatal(err) + } + job := e.waitJob(t, id) + if job.Kind != dinners.KindBackfillPhotos || string(job.Result) != `{"dinners":[]}` { + t.Fatalf("job = %+v (result %s), want a backfill with no dinners", job, job.Result) + } + if got := e.search.Queries()[searched:]; len(got) != 0 { + t.Errorf("backfill searched %q, want nothing", got) + } +} + +func TestBackfillOfOneDinnerGroundsOnlyIt(t *testing.T) { + t.Parallel() + e := newUnstartedEnv(t) + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + curry := e.failedDinner(t, "Chicken curry") + other := e.failedDinner(t, "Abalone surprise") + + got, err := e.svc.RunBackfillJob(t.Context(), dinners.BackfillJob{DinnerID: curry}) + if err != nil { + t.Fatal(err) + } + checkOutcomes(t, got, []dinners.BackfillOutcome{outcome(curry, dinners.BackfillGrounded, nil)}) + if row := e.dinner(t, other); row.GroundingStatus != dinners.GroundingFailed || row.PhotoID.Valid { + t.Errorf("other dinner = %+v, want left failed", row) + } + if got := e.search.Queries(); !slices.Equal(got, []string{"Chicken curry"}) { + t.Errorf("searched %q, want only the dinner's name", got) + } +} + +func TestBackfillLeavesDinnerChangedWhileGrounding(t *testing.T) { + t.Parallel() + e := newUnstartedEnv(t) + id := e.failedDinner(t, "Chicken curry") + gate := make(chan struct{}) + e.search.Gate = gate + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + + done := make(chan []dinners.BackfillOutcome) + go func() { + got, err := e.svc.RunBackfillJob(t.Context(), dinners.BackfillJob{DinnerID: id}) + if err != nil { + t.Error(err) + } + done <- got + }() + // The backfill listed the dinner and waits at its search. + for len(e.search.Queries()) == 0 { + time.Sleep(time.Millisecond) + } + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Chicken korma")}, member); err != nil { + t.Fatal(err) + } + close(gate) + checkOutcomes(t, <-done, []dinners.BackfillOutcome{outcome(id, dinners.BackfillSkipped, nil)}) + if row := e.dinner(t, id); row.GroundingStatus != dinners.GroundingFailed || row.PhotoID.Valid { + t.Errorf("dinner = %+v, want left failed without a photo", row) + } +} + +// triggerAtOnce calls svc.StartPhotoBackfill from n goroutines released +// together and returns the job ids they got. +func triggerAtOnce(t *testing.T, svc *dinners.Service, n int) []string { + t.Helper() + ids := make([]string, n) + errs := make([]error, n) + release := make(chan struct{}) + var wg sync.WaitGroup + for i := range n { + wg.Add(1) + go func() { + defer wg.Done() + <-release + ids[i], errs[i] = svc.StartPhotoBackfill(t.Context()) + }() + } + close(release) + wg.Wait() + if err := errors.Join(errs...); err != nil { + t.Fatal(err) + } + return ids +} + +// backfillJobs counts the KindBackfillPhotos jobs in st. +func backfillJobs(t *testing.T, st *store.Store) int { + t.Helper() + var n int + if err := st.Reader.QueryRowContext(t.Context(), `SELECT count(*) FROM jobs WHERE kind = ?`, dinners.KindBackfillPhotos).Scan(&n); err != nil { + t.Fatal(err) + } + return n +} + +func TestStartPhotoBackfillIsSingleFlight(t *testing.T) { + t.Parallel() + const triggers = 8 + dataDir := t.TempDir() + st := openStore(t, dataDir) + e := newUnstartedEnvOn(t, st, dataDir) + e.failedDinner(t, "Chicken curry") + gate := make(chan struct{}) + e.search.Gate = gate + + // Queued: simultaneous triggers all get the one pass they enqueued. + ids := triggerAtOnce(t, e.svc, triggers) + first := ids[0] + for _, id := range ids { + if id != first { + t.Fatalf("simultaneous triggers = %q, want one job id", ids) + } + } + if n := backfillJobs(t, st); n != 1 { + t.Fatalf("%d backfill jobs after simultaneous triggers, want 1", n) + } + + // After a restart the queued pass is still the one: the claim is the + // stored job, not the process's memory. + if err := st.Close(); err != nil { + t.Fatal(err) + } + st = openStore(t, dataDir) + restarted := newUnstartedEnvOn(t, st, dataDir) + restarted.search.Gate = gate + restarted.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + if again, err := restarted.svc.StartPhotoBackfill(t.Context()); err != nil || again != first { + t.Fatalf("trigger after restart = %q, %v; want the queued %q", again, err, first) + } + + // Running: the pass waits at its search, and simultaneous triggers all + // return it. + restarted.start(t) + for len(restarted.search.Queries()) == 0 { + time.Sleep(time.Millisecond) + } + for _, id := range triggerAtOnce(t, restarted.svc, triggers) { + if id != first { + t.Fatalf("trigger while running = %q, want the running %q", id, first) + } + } + if n := backfillJobs(t, st); n != 1 { + t.Fatalf("%d backfill jobs, want 1", n) + } + + // Finished: the next trigger starts a new pass. + close(gate) + if job := restarted.waitJob(t, first); job.Status != jobs.StatusSucceeded { + t.Fatalf("job = %+v (result %s), want succeeded", job, job.Result) + } + next, err := restarted.svc.StartPhotoBackfill(t.Context()) + if err != nil { + t.Fatal(err) + } + if next == first { + t.Fatalf("trigger after the pass finished = %q, want a new job", next) + } + restarted.waitJob(t, next) +} + +func TestBackfillFailsJobWhenAWriteFails(t *testing.T) { + t.Parallel() + e := newEnv(t) + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + absent := e.failedDinner(t, "Abalone surprise") + curry := e.failedDinner(t, "Chicken curry") + if _, err := e.st.Writer.ExecContext(t.Context(), `CREATE TRIGGER refuse_grounded BEFORE UPDATE OF grounding_status ON dinners + WHEN NEW.id = '`+curry+`' AND NEW.grounding_status = 'grounded' + BEGIN SELECT RAISE(ABORT, 'grounded refused'); END`); err != nil { + t.Fatal(err) + } + + id, err := e.svc.StartPhotoBackfill(t.Context()) + if err != nil { + t.Fatal(err) + } + job := e.waitJob(t, id) + if job.Status != jobs.StatusFailed || job.Error == nil || *job.Error != jobs.CodeInternal { + t.Fatalf("job = %+v, want failed with %s", job, jobs.CodeInternal) + } + var got struct { + Dinners []dinners.BackfillOutcome `json:"dinners"` + } + if err := json.Unmarshal(job.Result, &got); err != nil { + t.Fatalf("result %s: %v", job.Result, err) + } + // The other dinner was still tried and recorded. + checkOutcomes(t, got.Dinners, []dinners.BackfillOutcome{ + outcome(absent, dinners.BackfillFailed, ptr(dinners.ReasonNoRecipe)), + outcome(curry, dinners.BackfillFailed, ptr(dinners.ReasonGroundingError)), + }) + if row := e.dinner(t, curry); row.GroundingStatus != dinners.GroundingFailed || row.PhotoID.Valid { + t.Errorf("curry = %+v, want left failed without a photo", row) + } +} diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 45308e5..e2a57ef 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -138,6 +138,8 @@ type Service struct { grounder *recipes.Grounder logger *slog.Logger groundJob *jobs.Kind[GroundJob] + // backfillJob enqueues KindBackfillPhotos jobs. + backfillJob *jobs.Kind[BackfillJob] // wake is called after every committed change to a dinner's method or // minutes, which can move its crock-pot start reminder. wake func() @@ -162,8 +164,8 @@ type Service struct { // NewService returns a service over st that grounds dinners with grounder, // calls wake (the scheduler's Wake) after every committed change to a -// dinner's method or minutes, and registers the KindGroundDinner job on -// runner, which must not have started yet. +// dinner's method or minutes, and registers the KindGroundDinner and +// KindBackfillPhotos jobs on runner, which must not have started yet. func NewService(st *store.Store, grounder *recipes.Grounder, runner *jobs.Runner, wake func(), logger *slog.Logger) (*Service, error) { switch { case st == nil: @@ -184,6 +186,14 @@ func NewService(st *store.Store, grounder *recipes.Grounder, runner *jobs.Runner return nil, err } s.groundJob = kind + backfill, err := jobs.Register(runner, KindBackfillPhotos, s.runBackfillJob) + if err != nil { + return nil, err + } + if err := backfill.ServiceOnly(); err != nil { + return nil, err + } + s.backfillJob = backfill return s, nil } diff --git a/internal/dinners/export_test.go b/internal/dinners/export_test.go index 0f4a731..dfdf5ee 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -74,3 +74,18 @@ func (s *Service) RunGroundJob(ctx context.Context, p GroundJob) (string, error) // SafeSourceURL is safeSourceURL. var SafeSourceURL = safeSourceURL + +// RunBackfillJob runs the KindBackfillPhotos handler for p in the caller's +// goroutine, as the job runner would, and returns its outcomes: on an +// error, those it last published. +func (s *Service) RunBackfillJob(ctx context.Context, p BackfillJob) ([]BackfillOutcome, error) { + var published backfillResult + r, err := s.runBackfillJob(ctx, p, func(partial backfillResult) error { + published = partial + return nil + }) + if err != nil { + return published.Dinners, err + } + return r.Dinners, nil +} diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 11fb4c0..890f2a9 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -39,6 +39,7 @@ type env struct { runner *jobs.Runner search *recipestest.Search pages *recipestest.Pages + images *recipestest.Images model *llm.Fake } @@ -108,7 +109,7 @@ func newUnstartedEnvOn(t *testing.T, st *store.Store, dataDir string) *env { if err != nil { t.Fatal(err) } - return &env{svc: svc, st: st, runner: runner, search: search, pages: pages, model: model} + return &env{svc: svc, st: st, runner: runner, search: search, pages: pages, images: images, model: model} } // start starts the env's job runner, stopping it when the test ends. diff --git a/internal/server/dinners.go b/internal/server/dinners.go index 5f0d54e..4934594 100644 --- a/internal/server/dinners.go +++ b/internal/server/dinners.go @@ -112,6 +112,20 @@ func postDinner(logger *slog.Logger, svc *dinners.Service) http.HandlerFunc { } } +// postPhotoBackfill starts a photo backfill over every dinner without a +// photo and answers 202 with its job id, which GET /jobs/{id} reports on, +// without waiting for the job. +func postPhotoBackfill(logger *slog.Logger, svc *dinners.Service) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + id, err := svc.StartPhotoBackfill(r.Context()) + if err != nil { + writeDinnerError(w, r, logger, "start photo backfill", err) + return + } + writeJSON(w, r, logger, http.StatusAccepted, jobCreated{ID: id}) + } +} + // patchDinner edits a dinner, recording the session's actor as updated_by. func patchDinner(logger *slog.Logger, svc *dinners.Service) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/server/grounding_test.go b/internal/server/grounding_test.go index dfac21a..5be3a66 100644 --- a/internal/server/grounding_test.go +++ b/internal/server/grounding_test.go @@ -556,3 +556,43 @@ func TestPostDinnerEnqueueFailureAnswersFailedDinner(t *testing.T) { t.Errorf("listed = %v, want failed", got) } } + +func TestPostPhotoBackfillGroundsFailedDinners(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t).withRecipes(t) + e.start(t) + + // No model reply is scripted, so grounding on creation fails. + rec, created := callDinners(t, e.h, http.MethodPost, dinnersPath, `{"name":"Chicken curry"}`) + if rec.Code != http.StatusCreated { + t.Fatalf("POST = %d: %s", rec.Code, rec.Body) + } + id := created["id"].(string) + waitJob(t, e.h, created["grounding_job_id"].(string)) + if d := listedDinner(t, e.h, id); d["grounding_status"] != "failed" || d["photo_url"] != nil { + t.Fatalf("dinner = %v, want failed without a photo", d) + } + + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + rec, started := callDinners(t, e.h, http.MethodPost, "/api/v1/settings/photo-backfill", "") + jobID, _ := started["id"].(string) + if rec.Code != http.StatusAccepted || jobID == "" { + t.Fatalf("POST photo-backfill = %d: %s, want 202 with a job id", rec.Code, rec.Body) + } + job := waitJob(t, e.h, jobID) + want := fmt.Sprintf(`map[dinners:[map[dinner_id:%s outcome:grounded reason:]]]`, id) + if job["status"] != "succeeded" || job["kind"] != "backfill_dinner_photos" || fmt.Sprint(job["result"]) != want { + t.Fatalf("job = %v, want backfill_dinner_photos succeeded with %s", job, want) + } + d := listedDinner(t, e.h, id) + photo, _ := d["photo_url"].(string) + if d["grounding_status"] != "grounded" || d["grounding_error"] != nil || !strings.HasSuffix(photo, "-400.jpg") { + t.Errorf("dinner = %v, want grounded with a photo", d) + } + + // The backfill is started only by its own endpoint. + rec, _ = callDinners(t, e.h, http.MethodPost, "/api/v1/jobs", `{"kind":"backfill_dinner_photos","payload":{}}`) + if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), "service_only_kind") { + t.Errorf("POST /jobs backfill = %d: %s, want 400 service_only_kind", rec.Code, rec.Body) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 3b0352e..c7fce9b 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -162,6 +162,7 @@ func NewRouter(logger *slog.Logger, version string, trusted forwarded.Trusted, a apiV1.Get("/settings/chat-webhook", getChatWebhook(logger, chatSvc)) apiV1.Put("/settings/chat-webhook", putChatWebhook(logger, chatSvc)) apiV1.Post("/settings/chat-webhook/test", postChatWebhookTest(logger, chatSvc)) + apiV1.Post("/settings/photo-backfill", postPhotoBackfill(logger, dinnerSvc)) apiV1.Get("/dinners", getDinners(logger, dinnerSvc)) apiV1.Post("/dinners", postDinner(logger, dinnerSvc)) apiV1.Get("/dinners/{id}", getDinner(logger, dinnerSvc)) diff --git a/migrations/queries/dinners.sql b/migrations/queries/dinners.sql index 98795bf..8bed5a9 100644 --- a/migrations/queries/dinners.sql +++ b/migrations/queries/dinners.sql @@ -181,3 +181,65 @@ WHERE grounding_status = 'pending' AND json_extract(jobs.payload, '$.dinner_id') = dinners.id AND json_extract(jobs.payload, '$.revision') = dinners.grounding_revision ); + +-- name: ListPhotoBackfillDinners :many +-- The dinners a photo backfill grounds by name: unarchived, failed, with no +-- photo and no pasted recipe_url (the household's own link is never +-- replaced by a search), ordered by normalized name. A non-NULL id narrows +-- the list to that dinner. Grounded, manual and pending dinners are never +-- listed: they have a recipe, or a ground_dinner job grounding them. +SELECT * FROM dinners +WHERE status <> 'archived' + AND grounding_status = 'failed' + AND photo_id IS NULL + AND recipe_url IS NULL + AND id = COALESCE(sqlc.narg(id), id) +ORDER BY normalized_name, id; + +-- name: GetActivePhotoBackfillJob :one +-- The id of the full photo backfill job (no dinner_id) of kind queued or +-- running. StartPhotoBackfill enqueues one only when there is none, so +-- there is at most one. Returns sql.ErrNoRows when there is none. +SELECT id +FROM jobs +WHERE kind = sqlc.arg(kind) + AND status IN ('queued', 'running') + AND json_extract(payload, '$.dinner_id') IS NULL +ORDER BY id +LIMIT 1; + +-- name: GroundFailedDinner :execrows +-- Records a photo backfill's grounding of a dinner it listed, while the +-- dinner is still unarchived, failed, photoless, without a recipe_url and at +-- the grounding_revision it was listed at; a dinner grounded by hand, +-- renamed or archived meanwhile is left as it is (0 rows). Like +-- GroundPendingDinner it keeps a method, minutes_total or difficulty a +-- member already set. +UPDATE dinners +SET recipe_id = sqlc.arg(recipe_id), + photo_id = sqlc.arg(photo_id), + method = COALESCE(method, sqlc.arg(method)), + minutes_total = COALESCE(minutes_total, sqlc.arg(minutes_total)), + difficulty = COALESCE(difficulty, sqlc.arg(difficulty)), + grounding_status = 'grounded', + grounding_error = NULL, + updated_at = sqlc.arg(updated_at) +WHERE id = sqlc.arg(id) + AND status <> 'archived' + AND grounding_status = 'failed' + AND photo_id IS NULL + AND recipe_url IS NULL + AND grounding_revision = sqlc.arg(grounding_revision); + +-- name: RefailDinnerGrounding :execrows +-- Records why a photo backfill could not ground a dinner it listed, under +-- the same conditions as GroundFailedDinner (0 rows otherwise). +UPDATE dinners +SET grounding_error = sqlc.arg(grounding_error), + updated_at = sqlc.arg(updated_at) +WHERE id = sqlc.arg(id) + AND status <> 'archived' + AND grounding_status = 'failed' + AND photo_id IS NULL + AND recipe_url IS NULL + AND grounding_revision = sqlc.arg(grounding_revision);