diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9ee7df7..9794d18 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -250,7 +250,14 @@ automatic** (D-021). `search_failed` / `ai_failed` (only the reason code is logged, never the provider's text), and grounding a candidate or link that runs past 90 seconds (fetch, photo and `normalize_recipe`, all in the request) 504 - `ground_timeout`, leaving the dinner unchanged. + `ground_timeout`, leaving the dinner unchanged. An archived dinner is 409 + `dinner_archived`. A candidate or link is written only if the dinner is + still as it was before grounding began: one renamed or grounded some + other way meanwhile is 409 `ground_conflict`, and one archived meanwhile + 409 `dinner_archived`, either leaving the dinner unchanged. Any other + failure grounding a candidate or link is 500, logged with a fixed reason + and the dinner id only — never the link, which can carry a personal + token, or dependency error text. - `/health` — liveness. - Everything else serves the embedded PWA with SPA fallback. diff --git a/internal/db/dinners.sql.go b/internal/db/dinners.sql.go index ff1200e..83457a3 100644 --- a/internal/db/dinners.sql.go +++ b/internal/db/dinners.sql.go @@ -248,35 +248,48 @@ func (q *Queries) GetDinner(ctx context.Context, id string) (Dinner, error) { const groundDinner = `-- name: GroundDinner :one UPDATE dinners -SET recipe_id = ?, +SET recipe_id = ?1, recipe_url = NULL, - photo_id = ?, - method = ?, - minutes_total = ?, - difficulty = ?, - grounding_status = ?, + photo_id = ?2, + method = ?3, + minutes_total = ?4, + difficulty = ?5, + grounding_status = ?6, grounding_error = NULL, - updated_by = ?, - updated_at = ? -WHERE id = ? + updated_by = ?7, + updated_at = ?8 +WHERE id = ?9 + AND status <> 'archived' + AND normalized_name = ?10 + AND grounding_status = ?11 + AND grounding_revision = ?12 + AND recipe_id IS ?13 RETURNING 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 ` type GroundDinnerParams struct { - RecipeID sql.NullString - PhotoID sql.NullString - Method sql.NullString - MinutesTotal sql.NullInt64 - Difficulty sql.NullString - GroundingStatus string - UpdatedBy sql.NullString - UpdatedAt string - ID string + RecipeID sql.NullString + PhotoID sql.NullString + Method sql.NullString + MinutesTotal sql.NullInt64 + Difficulty sql.NullString + GroundingStatus string + UpdatedBy sql.NullString + UpdatedAt string + ID string + ObservedNormalizedName string + ObservedGroundingStatus string + ObservedGroundingRevision int64 + ObservedRecipeID sql.NullString } // Records a dinner's grounding by hand (grounded or manual) and clears any // grounding_error and pasted recipe_url: the recipe chosen replaces the -// link. An unknown id returns sql.ErrNoRows. +// link. It writes only an unarchived dinner still as it was observed +// before grounding (same normalized_name, grounding_status, +// grounding_revision and recipe_id), so a dinner renamed, archived or +// grounded some other way meanwhile is left as it is. That dinner, or an +// unknown id, returns sql.ErrNoRows. func (q *Queries) GroundDinner(ctx context.Context, arg GroundDinnerParams) (Dinner, error) { row := q.db.QueryRowContext(ctx, groundDinner, arg.RecipeID, @@ -288,6 +301,10 @@ func (q *Queries) GroundDinner(ctx context.Context, arg GroundDinnerParams) (Din arg.UpdatedBy, arg.UpdatedAt, arg.ID, + arg.ObservedNormalizedName, + arg.ObservedGroundingStatus, + arg.ObservedGroundingRevision, + arg.ObservedRecipeID, ) var i Dinner err := row.Scan( diff --git a/internal/db/querier.go b/internal/db/querier.go index 382e233..0d8f5d6 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -69,7 +69,11 @@ type Querier interface { GetRecipeSearchSettings(ctx context.Context) (RecipeSearchSetting, error) // Records a dinner's grounding by hand (grounded or manual) and clears any // grounding_error and pasted recipe_url: the recipe chosen replaces the - // link. An unknown id returns sql.ErrNoRows. + // link. It writes only an unarchived dinner still as it was observed + // before grounding (same normalized_name, grounding_status, + // grounding_revision and recipe_id), so a dinner renamed, archived or + // grounded some other way meanwhile is left as it is. That dinner, or an + // unknown id, returns sql.ErrNoRows. GroundDinner(ctx context.Context, arg GroundDinnerParams) (Dinner, error) // Records the background grounding of an unarchived dinner still pending at // the grounding_revision it was grounded for; a dinner grounded some other diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index 9fd4115..4bf7e70 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -76,6 +76,18 @@ var groundTimeout = GroundTimeout // runs past GroundTimeout; the dinner is left as it was. var ErrGroundTimeout = errors.New("dinners: grounding the recipe took too long") +// ErrGroundFailed wraps any failure grounding a candidate or link by hand. +// Its text may quote the link and dependency errors, so it is never +// logged. +var ErrGroundFailed = errors.New("dinners: grounding the recipe failed") + +// ErrArchived is grounding an archived dinner by hand. +var ErrArchived = errors.New("dinner archived") + +// ErrGroundConflict is a candidate or link grounded for a dinner that was +// renamed or grounded some other way while it was grounding. +var ErrGroundConflict = errors.New("dinner changed while its recipe was grounding") + // GroundJob is the KindGroundDinner payload: the dinner to ground and its // grounding_revision when the job was enqueued. type GroundJob struct { @@ -326,8 +338,11 @@ type ManualRecipe struct { // difficulty. A manual recipe is stored as typed — no model runs — and // sets grounding_status manual with its method and minutes; it has no // photo, and the dinner keeps its difficulty. An invalid f is a -// *ValidationError; a page that cannot ground the dinner is a -// *recipes.ErrUngroundable and changes nothing. +// *ValidationError and an archived dinner ErrArchived. A failure grounding +// a candidate or link wraps ErrGroundFailed; a page that cannot ground the +// dinner is also a *recipes.ErrUngroundable. A dinner renamed, archived or +// grounded some other way while a candidate or link was grounding is +// ErrGroundConflict or ErrArchived. None of these changes the dinner. func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string) (Dinner, error) { if updatedBy == "" { return Dinner{}, errors.New("dinners: ground: no member to record as updated_by") @@ -339,9 +354,13 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string if manual != nil { return s.groundManual(ctx, id, *manual, updatedBy) } - if _, err := s.get(ctx, id); err != nil { + cur, err := s.get(ctx, id) + if err != nil { return Dinner{}, err } + if cur.Status == StatusArchived { + return Dinner{}, ErrArchived + } groundCtx, cancel := context.WithTimeout(ctx, groundTimeout) defer cancel() gr, err := s.grounder.GroundURL(groundCtx, link) @@ -349,21 +368,25 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string if ctx.Err() == nil && errors.Is(groundCtx.Err(), context.DeadlineExceeded) { return Dinner{}, ErrGroundTimeout } - return Dinner{}, fmt.Errorf("dinners: ground %s: %w", id, err) + return Dinner{}, fmt.Errorf("%w: dinner %s: %w", ErrGroundFailed, id, err) } row, err := db.New(s.st.Writer).GroundDinner(ctx, db.GroundDinnerParams{ - 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}, - GroundingStatus: GroundingGrounded, - UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, - UpdatedAt: store.Now().String(), - ID: id, + 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}, + GroundingStatus: GroundingGrounded, + UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, + UpdatedAt: store.Now().String(), + ID: cur.ID, + ObservedNormalizedName: cur.NormalizedName, + ObservedGroundingStatus: cur.GroundingStatus, + ObservedGroundingRevision: cur.GroundingRevision, + ObservedRecipeID: cur.RecipeID, }) if errors.Is(err, sql.ErrNoRows) { - return Dinner{}, ErrNotFound + return Dinner{}, s.groundRejected(ctx, id) } if err != nil { return Dinner{}, fmt.Errorf("dinners: ground %s: %w", id, err) @@ -371,6 +394,19 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string return fromRow(row), nil } +// groundRejected is the error for dinner id when it was not as observed +// before grounding: ErrNotFound, ErrArchived or ErrGroundConflict. +func (s *Service) groundRejected(ctx context.Context, id string) error { + cur, err := s.get(ctx, id) + if err != nil { + return err + } + if cur.Status == StatusArchived { + return ErrArchived + } + return ErrGroundConflict +} + // groundManual stores m as dinner id's recipe, with its ingredients and // steps, and records it on the dinner, all in one transaction. func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, updatedBy string) (Dinner, error) { @@ -384,6 +420,9 @@ func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, u if err != nil { return fmt.Errorf("dinners: read %s: %w", id, err) } + if cur.Status == StatusArchived { + return ErrArchived + } ld, err := json.Marshal(manualJSONLD{ Context: "https://schema.org", Type: "Recipe", @@ -439,14 +478,18 @@ func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, u } } row, err = q.GroundDinner(ctx, db.GroundDinnerParams{ - RecipeID: sql.NullString{String: rec.ID, Valid: true}, - Method: method, - MinutesTotal: minutes, - Difficulty: cur.Difficulty, - GroundingStatus: GroundingManual, - UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, - UpdatedAt: now, - ID: cur.ID, + RecipeID: sql.NullString{String: rec.ID, Valid: true}, + Method: method, + MinutesTotal: minutes, + Difficulty: cur.Difficulty, + GroundingStatus: GroundingManual, + UpdatedBy: sql.NullString{String: updatedBy, Valid: true}, + UpdatedAt: now, + ID: cur.ID, + ObservedNormalizedName: cur.NormalizedName, + ObservedGroundingStatus: cur.GroundingStatus, + ObservedGroundingRevision: cur.GroundingRevision, + ObservedRecipeID: cur.RecipeID, }) if err != nil { return fmt.Errorf("dinners: ground %s: %w", id, err) diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 2e14546..1f837a7 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -1106,3 +1106,103 @@ func TestGroundTimesOut(t *testing.T) { t.Errorf("grounding_status = %s, want failed as before", row.GroundingStatus) } } + +// TestFixInFlightWhenDinnerChanges: a link still grounding when its dinner +// is renamed, archived or grounded some other way writes nothing; the +// change made meanwhile is kept. +func TestFixInFlightWhenDinnerChanges(t *testing.T) { + t.Parallel() + tests := []struct { + name string + change func(t *testing.T, e *env, id string) + want error + // kept checks the dinner shows the change, not the link's recipe. + kept func(row db.Dinner) bool + }{ + {"renamed", func(t *testing.T, e *env, id string) { + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Chicken curry")}, member); err != nil { + t.Fatal(err) + } + }, dinners.ErrGroundConflict, func(row db.Dinner) bool { + return row.Name == "Chicken curry" && row.GroundingStatus == dinners.GroundingFailed && !row.RecipeID.Valid + }}, + {"archived", func(t *testing.T, e *env, id string) { + if _, err := e.svc.Archive(t.Context(), id, member); err != nil { + t.Fatal(err) + } + }, dinners.ErrArchived, func(row db.Dinner) bool { + return row.Status == dinners.StatusArchived && row.GroundingStatus == dinners.GroundingFailed && !row.RecipeID.Valid + }}, + {"grounded by hand", func(t *testing.T, e *env, id string) { + if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{Manual: &dinners.ManualRecipe{ + Ingredients: []string{"1 lb beef"}, Minutes: ptr(240), Method: ptr("crockpot"), + }}, member); err != nil { + t.Fatal(err) + } + }, dinners.ErrGroundConflict, func(row db.Dinner) bool { + return row.GroundingStatus == dinners.GroundingManual && row.Method.String == "crockpot" + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + e := newEnv(t) + id := failedDinner(t, e, "Grandma's stew") + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + e.pages.Gate = make(chan struct{}) + + done := make(chan error, 1) + go func() { + _, err := e.svc.Ground(context.Background(), id, dinners.Fix{RecipeURL: ptr(stew)}, member) + done <- err + }() + e.waitFetched(t, stew) + tt.change(t, e, id) + close(e.pages.Gate) + + if err := <-done; !errors.Is(err, tt.want) { + t.Fatalf("err = %v, want %v", err, tt.want) + } + if row := e.dinner(t, id); !tt.kept(row) { + t.Errorf("dinner = %+v, want the %s change kept and the link's recipe not written", row, tt.name) + } + }) + } +} + +// TestFixArchivedDinner: an archived dinner cannot be grounded by hand. +func TestFixArchivedDinner(t *testing.T) { + t.Parallel() + e := newEnv(t) + id := failedDinner(t, e, "Grandma's stew") + if _, err := e.svc.Archive(t.Context(), id, member); err != nil { + t.Fatal(err) + } + before := e.dinner(t, id) + for _, f := range []dinners.Fix{ + {RecipeURL: ptr(stew)}, + {Manual: &dinners.ManualRecipe{Ingredients: []string{"1 lb beef"}, Minutes: ptr(240), Method: ptr("crockpot")}}, + } { + if _, err := e.svc.Ground(t.Context(), id, f, member); !errors.Is(err, dinners.ErrArchived) { + t.Errorf("fix %+v: err = %v, want ErrArchived", f, err) + } + } + if got := e.pages.Fetched(); len(got) != 0 { + t.Errorf("fetched %q for an archived dinner", got) + } + if after := e.dinner(t, id); after != before { + t.Errorf("archived dinner changed: %+v, was %+v", after, before) + } +} + +// waitFetched waits until rawURL has been fetched. +func (e *env) waitFetched(t *testing.T, rawURL string) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for !slices.Contains(e.pages.Fetched(), rawURL) { + if time.Now().After(deadline) { + t.Fatalf("%s never fetched", rawURL) + } + time.Sleep(5 * time.Millisecond) + } +} diff --git a/internal/recipes/ground.go b/internal/recipes/ground.go index b5e5b16..6b14436 100644 --- a/internal/recipes/ground.go +++ b/internal/recipes/ground.go @@ -266,7 +266,7 @@ func (g *Grounder) GroundURL(ctx context.Context, rawURL string) (Grounded, erro } gr, reason, err := g.ground(ctx, rawURL, r) if err != nil { - return Grounded{}, fmt.Errorf("recipes: ground %s: %w", rawURL, err) + return Grounded{}, fmt.Errorf("recipes: ground pasted link: %w", err) } if reason != nil { return ungroundable(reason) diff --git a/internal/server/dinners.go b/internal/server/dinners.go index 185b823..fcb0197 100644 --- a/internal/server/dinners.go +++ b/internal/server/dinners.go @@ -200,7 +200,10 @@ func actorOrFail(w http.ResponseWriter, r *http.Request, logger *slog.Logger) (a // writeDinnerError maps a dinners.Service error to its API error. A page // that cannot ground the dinner is 422 ungroundable; recipe search not set // up is 409 not_configured; a search or model failure is 502; grounding by -// hand that runs past dinners.GroundTimeout is 504 ground_timeout. +// hand that runs past dinners.GroundTimeout is 504 ground_timeout; grounding +// an archived dinner is 409 dinner_archived and one changed while grounding +// 409 ground_conflict. Any other grounding failure is 500, logged by reason +// and dinner id only. func writeDinnerError(w http.ResponseWriter, r *http.Request, logger *slog.Logger, action string, err error) { var invalid *dinners.ValidationError var dup *dinners.DuplicateError @@ -220,6 +223,16 @@ func writeDinnerError(w http.ResponseWriter, r *http.Request, logger *slog.Logge case errors.Is(err, recipes.ErrNormalizeFailed): logger.WarnContext(r.Context(), action, "reason", "normalize_recipe failed") writeError(w, r, logger, http.StatusBadGateway, "ai_failed", "the AI provider failed to read the recipe") + case errors.Is(err, dinners.ErrGroundFailed): + // The error may quote the pasted link, which can carry a personal + // token, and dependency errors, so only the reason and dinner are + // logged. + logger.ErrorContext(r.Context(), action, "reason", "ground_failed", "dinner_id", chi.URLParam(r, "id")) + writeError(w, r, logger, http.StatusInternalServerError, "internal", "internal error") + case errors.Is(err, dinners.ErrArchived): + writeError(w, r, logger, http.StatusConflict, "dinner_archived", "the dinner is archived") + case errors.Is(err, dinners.ErrGroundConflict): + writeError(w, r, logger, http.StatusConflict, "ground_conflict", "the dinner changed while its recipe was grounding; reload and try again") case errors.Is(err, dinners.ErrGroundTimeout): logger.WarnContext(r.Context(), action, "reason", "ground_timeout") writeError(w, r, logger, http.StatusGatewayTimeout, "ground_timeout", "grounding the recipe took too long; try again") diff --git a/internal/server/grounding_test.go b/internal/server/grounding_test.go index 1365f9e..9d3747d 100644 --- a/internal/server/grounding_test.go +++ b/internal/server/grounding_test.go @@ -224,6 +224,49 @@ func TestPostGroundViaRecipeURL(t *testing.T) { } } +// TestPostGroundViaRecipeURLStorageFailure: a pasted link that fails for a +// reason other than its recipe is a 500 logged by reason and dinner id, +// never with the link, which can carry a personal token, or the dependency +// error. +func TestPostGroundViaRecipeURLStorageFailure(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t).withRecipes(t) + const token = "personal-token-abc123" + link := stewPage + "?access_token=" + token + e.pages.Bodies[link] = e.pages.Bodies[stewPage] + id := createDinner(t, e.h, `{"name":"Sunday stew"}`) + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + if _, err := e.st.Writer.ExecContext(t.Context(), + `CREATE TRIGGER refuse_recipes BEFORE INSERT ON recipes BEGIN SELECT RAISE(ABORT, 'disk full'); END`); err != nil { + t.Fatal(err) + } + + rec, body := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", `{"recipe_url":"`+link+`"}`) + if rec.Code != http.StatusInternalServerError || errorCode(body) != "internal" || strings.Contains(rec.Body.String(), token) { + t.Errorf("POST = %d %s, want 500 internal without the link", rec.Code, rec.Body) + } + logs := e.logs.String() + if strings.Contains(logs, token) || strings.Contains(logs, "c.example") || strings.Contains(logs, "disk full") || + !strings.Contains(logs, "reason=ground_failed") || !strings.Contains(logs, "dinner_id="+id) { + t.Errorf("logs = %q, want reason=ground_failed and the dinner id, without the link or dependency error", logs) + } +} + +// TestPostGroundArchived: grounding an archived dinner by hand is a 409. +func TestPostGroundArchived(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t) + id := createDinner(t, e.h, `{"name":"Family chili"}`) + if rec, _ := callDinners(t, e.h, http.MethodDelete, dinnersPath+"/"+id, ""); rec.Code != http.StatusOK { + t.Fatalf("DELETE = %d: %s", rec.Code, rec.Body) + } + rec, body := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", + `{"manual":{"ingredients":["1 lb beef"],"minutes":240,"method":"crockpot"}}`) + if rec.Code != http.StatusConflict || errorCode(body) != "dinner_archived" { + t.Errorf("POST = %d %v, want 409 dinner_archived", rec.Code, body) + } +} + func TestPostGroundViaManual(t *testing.T) { t.Parallel() e := newDinnersEnv(t) diff --git a/migrations/queries/dinners.sql b/migrations/queries/dinners.sql index e11dec1..93ed6e3 100644 --- a/migrations/queries/dinners.sql +++ b/migrations/queries/dinners.sql @@ -84,19 +84,28 @@ RETURNING *; -- name: GroundDinner :one -- Records a dinner's grounding by hand (grounded or manual) and clears any -- grounding_error and pasted recipe_url: the recipe chosen replaces the --- link. An unknown id returns sql.ErrNoRows. +-- link. It writes only an unarchived dinner still as it was observed +-- before grounding (same normalized_name, grounding_status, +-- grounding_revision and recipe_id), so a dinner renamed, archived or +-- grounded some other way meanwhile is left as it is. That dinner, or an +-- unknown id, returns sql.ErrNoRows. UPDATE dinners -SET recipe_id = ?, +SET recipe_id = sqlc.arg(recipe_id), recipe_url = NULL, - photo_id = ?, - method = ?, - minutes_total = ?, - difficulty = ?, - grounding_status = ?, + photo_id = sqlc.arg(photo_id), + method = sqlc.arg(method), + minutes_total = sqlc.arg(minutes_total), + difficulty = sqlc.arg(difficulty), + grounding_status = sqlc.arg(grounding_status), grounding_error = NULL, - updated_by = ?, - updated_at = ? -WHERE id = ? + updated_by = sqlc.arg(updated_by), + updated_at = sqlc.arg(updated_at) +WHERE id = sqlc.arg(id) + AND status <> 'archived' + AND normalized_name = sqlc.arg(observed_normalized_name) + AND grounding_status = sqlc.arg(observed_grounding_status) + AND grounding_revision = sqlc.arg(observed_grounding_revision) + AND recipe_id IS sqlc.narg(observed_recipe_id) RETURNING *; -- name: GroundPendingDinner :execrows