From ec718024a19066e159ab9f6fd0cb8ca2e2a7f315 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 11 Oct 2026 20:53:58 -0400 Subject: [PATCH 1/3] Add photo backfill for failed dinners without a photo Task 36c40130-a69a-46ed-a047-2cdc5ee2d2e0 (Feature 23, task 1 of 2). A backfill_dinner_photos job grounds, by name with the existing recipes.Grounder.Ground, every unarchived dinner whose grounding failed and that has no photo and no pasted recipe_url. Ground already tries the ranked candidates best first and keeps the first whose photo caches and whose ingredients normalize, reusing stored recipes and photos. Success records the recipe and photo as grounded; a failure leaves the dinner failed with the new grounding_error for the fix-recipe flow, without stopping its siblings. Writes are guarded on status, photo and grounding_revision, so grounded, manual, pending, renamed and archived dinners are never touched. POST /api/v1/settings/photo-backfill starts a full pass and answers 202 with the job id; the kind is service-only. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 13 ++ internal/db/dinners.sql.go | 144 +++++++++++++++++ internal/db/querier.go | 16 ++ internal/dinners/backfill.go | 145 +++++++++++++++++ internal/dinners/backfill_test.go | 241 +++++++++++++++++++++++++++++ internal/dinners/dinners.go | 14 +- internal/dinners/export_test.go | 7 + 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 | 50 ++++++ 12 files changed, 685 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..1d12443 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -259,6 +259,19 @@ 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; 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`. `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..6e629a4 100644 --- a/internal/db/dinners.sql.go +++ b/internal/db/dinners.sql.go @@ -344,6 +344,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 +496,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 +670,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..4ce3309 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -175,6 +175,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 +213,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 +320,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..96ac308 --- /dev/null +++ b/internal/dinners/backfill.go @@ -0,0 +1,145 @@ +package dinners + +import ( + "context" + "database/sql" + "encoding/json" + "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. +func (s *Service) StartPhotoBackfill(ctx context.Context) (string, error) { + id, err := s.backfillJob.Enqueue(ctx, BackfillJob{}) + if err != nil { + return "", fmt.Errorf("dinners: enqueue photo backfill: %w", err) + } + return id, nil +} + +// 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. Only a failure to list the dinners, or the job's context +// ending, fails the job. +func (s *Service) runBackfillJob(ctx context.Context, p BackfillJob, _ 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 := false + for _, row := range rows { + out, err := s.backfillDinner(ctx, row) + if err != nil { + return backfillResult{}, err + } + grounded = grounded || out.Outcome == BackfillGrounded + res.Dinners = append(res.Dinners, out) + } + if grounded { + s.wake() + } + return res, nil +} + +// backfillDinner grounds row, a dinner ListPhotoBackfillDinners listed, as +// runBackfillJob describes. Its error is only the job's context ending. +func (s *Service) backfillDinner(ctx context.Context, row db.Dinner) (BackfillOutcome, 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{}, 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, + }) + return s.backfillRecorded(ctx, out, BackfillFailed, reason, n, err), 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, + }) + return s.backfillRecorded(ctx, out, BackfillGrounded, "", n, err), 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. +func (s *Service) backfillRecorded(ctx context.Context, out BackfillOutcome, outcome, reason string, n int64, err error) BackfillOutcome { + 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 +} diff --git a/internal/dinners/backfill_test.go b/internal/dinners/backfill_test.go new file mode 100644 index 0000000..3ad08a4 --- /dev/null +++ b/internal/dinners/backfill_test.go @@ -0,0 +1,241 @@ +package dinners_test + +import ( + "database/sql" + "encoding/json" + "errors" + "slices" + "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/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) + } +} 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..ae83d86 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -74,3 +74,10 @@ 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. +func (s *Service) RunBackfillJob(ctx context.Context, p BackfillJob) ([]BackfillOutcome, error) { + r, err := s.runBackfillJob(ctx, p, nil) + return r.Dinners, err +} 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..f541136 100644 --- a/migrations/queries/dinners.sql +++ b/migrations/queries/dinners.sql @@ -181,3 +181,53 @@ 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: 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); -- 2.55.0 From 7c0eb5cc3aff0a3edbb7c4a60df3db7f5889a3c2 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 11 Oct 2026 21:16:13 -0400 Subject: [PATCH 2/3] Make photo backfill single-flight and fail on unrecorded writes Task 36c40130-a69a-46ed-a047-2cdc5ee2d2e0, remediation of review findings F23-1 and F23-2 (PR #205). F23-1: StartPhotoBackfill now checks, in the same write transaction that enqueues, for a full backfill_dinner_photos job already queued or running (new GetActivePhotoBackfillJob query) and returns its id instead of enqueuing another, as plan.StartDraft does with GetActiveDraftJob. The claim is the stored job row, so it holds across restarts; a job left running by a stopped process is failed at boot and no longer blocks. F23-2: a dinner whose outcome write fails is still reported failed with grounding_error and the remaining dinners still run, but the job then fails (internal_error, as a ground_dinner job's failed write does) with the per-dinner outcomes published as its result, so it never reports success for state it did not store. Tests: overlapping triggers while queued, after a restart and while running return the one job; a refused dinner write fails the job. Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 8 ++- internal/db/dinners.sql.go | 20 +++++++ internal/db/querier.go | 4 ++ internal/dinners/backfill.go | 75 ++++++++++++++++++------ internal/dinners/backfill_test.go | 95 +++++++++++++++++++++++++++++++ internal/dinners/export_test.go | 14 ++++- migrations/queries/dinners.sql | 12 ++++ 7 files changed, 207 insertions(+), 21 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1d12443..9ab4786 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -261,7 +261,9 @@ automatic** (D-021). 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; the kind is refused by `POST /api/v1/jobs`. The job + 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 @@ -271,7 +273,9 @@ automatic** (D-021). 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`. + `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 6e629a4..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 = ? diff --git a/internal/db/querier.go b/internal/db/querier.go index 4ce3309..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) diff --git a/internal/dinners/backfill.go b/internal/dinners/backfill.go index 96ac308..0c06c00 100644 --- a/internal/dinners/backfill.go +++ b/internal/dinners/backfill.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "encoding/json" + "errors" "fmt" "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db" @@ -47,15 +48,43 @@ type backfillResult struct { 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. +// 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) { - id, err := s.backfillJob.Enqueue(ctx, BackfillJob{}) + 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 "", fmt.Errorf("dinners: enqueue photo backfill: %w", err) + 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, @@ -64,38 +93,49 @@ func (s *Service) StartPhotoBackfill(ctx context.Context) (string, error) { // 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. Only a failure to list the dinners, or the job's context -// ending, fails the job. -func (s *Service) runBackfillJob(ctx context.Context, p BackfillJob, _ func(backfillResult) error) (backfillResult, error) { +// 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 := false + grounded, unrecorded := false, false for _, row := range rows { - out, err := s.backfillDinner(ctx, row) + 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. Its error is only the job's context ending. -func (s *Service) backfillDinner(ctx context.Context, row db.Dinner) (BackfillOutcome, error) { +// 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{}, ctxErr + return BackfillOutcome{}, false, ctxErr } if err != nil { reason := reasonFor(err, false) @@ -108,7 +148,8 @@ func (s *Service) backfillDinner(ctx context.Context, row db.Dinner) (BackfillOu ID: row.ID, GroundingRevision: row.GroundingRevision, }) - return s.backfillRecorded(ctx, out, BackfillFailed, reason, n, err), nil + 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}, @@ -120,14 +161,16 @@ func (s *Service) backfillDinner(ctx context.Context, row db.Dinner) (BackfillOu ID: row.ID, GroundingRevision: row.GroundingRevision, }) - return s.backfillRecorded(ctx, out, BackfillGrounded, "", n, err), nil + 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. -func (s *Service) backfillRecorded(ctx context.Context, out BackfillOutcome, outcome, reason string, n int64, err error) BackfillOutcome { +// 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. @@ -141,5 +184,5 @@ func (s *Service) backfillRecorded(ctx context.Context, out BackfillOutcome, out if reason != "" { out.Reason = &reason } - return out + return out, err == nil } diff --git a/internal/dinners/backfill_test.go b/internal/dinners/backfill_test.go index 3ad08a4..f82e58e 100644 --- a/internal/dinners/backfill_test.go +++ b/internal/dinners/backfill_test.go @@ -11,6 +11,7 @@ import ( "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" @@ -239,3 +240,97 @@ func TestBackfillLeavesDinnerChangedWhileGrounding(t *testing.T) { t.Errorf("dinner = %+v, want left failed without a photo", row) } } + +func TestStartPhotoBackfillIsSingleFlight(t *testing.T) { + t.Parallel() + dataDir := t.TempDir() + st := openStore(t, dataDir) + e := newUnstartedEnvOn(t, st, dataDir) + e.failedDinner(t, "Chicken curry") + e.search.Gate = make(chan struct{}) + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + + // Queued: a second trigger returns the queued pass. + first, err := e.svc.StartPhotoBackfill(t.Context()) + if err != nil { + t.Fatal(err) + } + if again, err := e.svc.StartPhotoBackfill(t.Context()); err != nil || again != first { + t.Fatalf("second trigger = %q, %v; want the queued %q", again, err, first) + } + + // After a restart the queued pass is still the one: the claim is the + // stored job, not the process's memory. + restarted := newUnstartedEnvOn(t, st, dataDir) + restarted.search.Gate = e.search.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 a trigger returns it. + restarted.start(t) + for len(restarted.search.Queries()) == 0 { + time.Sleep(time.Millisecond) + } + if again, err := restarted.svc.StartPhotoBackfill(t.Context()); err != nil || again != first { + t.Fatalf("trigger while running = %q, %v; want the running %q", again, err, first) + } + 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) + } + if n != 1 { + t.Fatalf("%d backfill jobs, want 1", n) + } + + // Finished: the next trigger starts a new pass. + close(e.search.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/export_test.go b/internal/dinners/export_test.go index ae83d86..dfdf5ee 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -76,8 +76,16 @@ func (s *Service) RunGroundJob(ctx context.Context, p GroundJob) (string, error) var SafeSourceURL = safeSourceURL // RunBackfillJob runs the KindBackfillPhotos handler for p in the caller's -// goroutine, as the job runner would, and returns its outcomes. +// 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) { - r, err := s.runBackfillJob(ctx, p, nil) - return r.Dinners, err + 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/migrations/queries/dinners.sql b/migrations/queries/dinners.sql index f541136..8bed5a9 100644 --- a/migrations/queries/dinners.sql +++ b/migrations/queries/dinners.sql @@ -196,6 +196,18 @@ WHERE status <> 'archived' 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 -- 2.55.0 From 32453d552b7bc1aa3b6db8cb4ba8b95c5cc01019 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 11 Oct 2026 21:34:09 -0400 Subject: [PATCH 3/3] Test photo backfill single-flight under simultaneous triggers TestStartPhotoBackfillIsSingleFlight now releases eight StartPhotoBackfill calls together behind a start gate, both while the pass is queued and while it runs, and asserts they all return the one job id with a single backfill job stored. The restart case closes the store and reopens it from the data directory before triggering again. Maestro task 36c40130-a69a-46ed-a047-2cdc5ee2d2e0 (review finding F23-1-T). Co-Authored-By: Claude Opus 5.5 --- internal/dinners/backfill_test.go | 78 ++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 18 deletions(-) diff --git a/internal/dinners/backfill_test.go b/internal/dinners/backfill_test.go index f82e58e..6d22243 100644 --- a/internal/dinners/backfill_test.go +++ b/internal/dinners/backfill_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "slices" + "sync" "testing" "time" @@ -241,51 +242,92 @@ func TestBackfillLeavesDinnerChangedWhileGrounding(t *testing.T) { } } +// 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") - e.search.Gate = make(chan struct{}) - e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + gate := make(chan struct{}) + e.search.Gate = gate - // Queued: a second trigger returns the queued pass. - first, err := e.svc.StartPhotoBackfill(t.Context()) - if err != nil { - t.Fatal(err) + // 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 again, err := e.svc.StartPhotoBackfill(t.Context()); err != nil || again != first { - t.Fatalf("second trigger = %q, %v; want the queued %q", again, err, first) + 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 = e.search.Gate + 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 a trigger returns it. + // 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) } - if again, err := restarted.svc.StartPhotoBackfill(t.Context()); err != nil || again != first { - t.Fatalf("trigger while running = %q, %v; want the running %q", again, err, first) + for _, id := range triggerAtOnce(t, restarted.svc, triggers) { + if id != first { + t.Fatalf("trigger while running = %q, want the running %q", id, first) + } } - 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) - } - if n != 1 { + if n := backfillJobs(t, st); n != 1 { t.Fatalf("%d backfill jobs, want 1", n) } // Finished: the next trigger starts a new pass. - close(e.search.Gate) + close(gate) if job := restarted.waitJob(t, first); job.Status != jobs.StatusSucceeded { t.Fatalf("job = %+v (result %s), want succeeded", job, job.Result) } -- 2.55.0