From 82f8f668bf4c53142f951e610615cef7d33ac38a Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 20:21:12 -0400 Subject: [PATCH] Log every unexpected recipe grounding failure by fixed reason Wrap an initial dinner read failure on the candidate and link path, and any manual-recipe read, store or write failure, in ErrGroundFailed so the handler logs only reason=ground_failed and the dinner id, never database text. Not-found, archived and conflict answers stay distinct. Strengthen TestFixInFlightKeepsMemberEdits: start from populated method, minutes and difficulty, edit each in flight in turn, and require the edit kept and the untouched populated fields replaced by the recipe's values. Co-Authored-By: Claude Opus 5.5 --- internal/dinners/grounding.go | 21 ++++-- internal/dinners/grounding_test.go | 110 ++++++++++++++++++++++------- internal/server/dinners.go | 3 +- internal/server/grounding_test.go | 39 ++++++++++ 4 files changed, 141 insertions(+), 32 deletions(-) diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index 19b0936..c978505 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -76,7 +76,7 @@ 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. +// ErrGroundFailed wraps any unexpected failure grounding a dinner 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") @@ -343,8 +343,9 @@ type ManualRecipe struct { // link, or recording it on the dinner, 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. +// was grounding is ErrGroundConflict or ErrArchived. A missing dinner is +// ErrNotFound; any other failure, by candidate, link or manual recipe, +// wraps ErrGroundFailed. 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") @@ -357,9 +358,12 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string return s.groundManual(ctx, id, *manual, updatedBy) } cur, err := s.get(ctx, id) - if err != nil { + if errors.Is(err, ErrNotFound) { return Dinner{}, err } + if err != nil { + return Dinner{}, fmt.Errorf("%w: dinner %s: %w", ErrGroundFailed, id, err) + } if cur.Status == StatusArchived { return Dinner{}, ErrArchived } @@ -418,7 +422,9 @@ func (s *Service) groundRejected(ctx context.Context, id string) error { } // groundManual stores m as dinner id's recipe, with its ingredients and -// steps, and records it on the dinner, all in one transaction. +// steps, and records it on the dinner, all in one transaction. A missing +// or archived dinner is ErrNotFound or ErrArchived; any other failure +// wraps ErrGroundFailed. func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, updatedBy string) (Dinner, error) { var row db.Dinner err := s.st.WithTx(ctx, func(tx *sql.Tx) error { @@ -510,9 +516,12 @@ func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, u } return nil }) - if err != nil { + if errors.Is(err, ErrNotFound) || errors.Is(err, ErrArchived) { return Dinner{}, err } + if err != nil { + return Dinner{}, fmt.Errorf("%w: dinner %s: %w", ErrGroundFailed, id, err) + } return fromRow(row), nil } diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 733f245..2578083 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -1177,38 +1177,98 @@ func TestFixInFlightWhenDinnerChanges(t *testing.T) { } } -// TestFixInFlightKeepsMemberEdits: a method or difficulty a member sets -// while a link is grounding is kept; what they left alone takes the link's -// recipe. +// TestFixInFlightKeepsMemberEdits: a method, minutes or difficulty a +// member sets while a link is grounding is kept; a field they left alone +// takes the link's recipe, even one already set before grounding began. func TestFixInFlightKeepsMemberEdits(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{}) + // Every field is set before grounding, then one is changed in flight. + before := dinners.Patch{Method: ptr("stovetop"), MinutesTotal: ptr(45), Difficulty: ptr("easy")} + tests := []struct { + name string + edit dinners.Patch + field string + }{ + {"method", dinners.Patch{Method: ptr("grill")}, "method"}, + {"minutes", dinners.Patch{MinutesTotal: ptr(20)}, "minutes_total"}, + {"difficulty", dinners.Patch{Difficulty: ptr("medium")}, "difficulty"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + e := newEnv(t) + id := failedDinner(t, e, "Grandma's stew") + if _, err := e.svc.Update(t.Context(), id, before, member); err != nil { + t.Fatal(err) + } + 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) - if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Method: ptr("grill"), Difficulty: ptr("easy")}, member); err != nil { - t.Fatal(err) - } - close(e.pages.Gate) + 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) + if _, err := e.svc.Update(t.Context(), id, tt.edit, member); err != nil { + t.Fatal(err) + } + close(e.pages.Gate) - if err := <-done; err != nil { - t.Fatal(err) + if err := <-done; err != nil { + t.Fatal(err) + } + row := e.dinner(t, id) + if row.GroundingStatus != dinners.GroundingGrounded || !row.RecipeID.Valid { + t.Fatalf("dinner = %+v, want grounded in the link's recipe", row) + } + var recipe fields + if err := e.st.Reader.QueryRowContext(t.Context(), + `SELECT method, minutes_total, difficulty FROM recipes WHERE id = ?`, row.RecipeID.String, + ).Scan(&recipe.method, &recipe.minutes, &recipe.difficulty); err != nil { + t.Fatal(err) + } + old, edited := patchFields(before), patchFields(tt.edit) + want := recipe + switch tt.field { + case "method": + want.method = edited.method + case "minutes_total": + want.minutes = edited.minutes + case "difficulty": + want.difficulty = edited.difficulty + } + if recipe.method == old.method || recipe.minutes == old.minutes || recipe.difficulty == old.difficulty { + t.Fatalf("recipe %+v shares a value with the dinner before grounding %+v", recipe, old) + } + got := fields{row.Method.String, row.MinutesTotal.Int64, row.Difficulty.String} + if got != want { + t.Errorf("method, minutes, difficulty = %+v, want %+v: the %s edit kept, the rest from the recipe", got, want, tt.field) + } + }) } - row := e.dinner(t, id) - if row.GroundingStatus != dinners.GroundingGrounded || !row.RecipeID.Valid { - t.Errorf("dinner = %+v, want grounded in the link's recipe", row) +} + +// fields are a dinner's or recipe's method, minutes_total and difficulty. +type fields struct { + method string + minutes int64 + difficulty string +} + +// patchFields returns the fields p sets, zero where it sets none. +func patchFields(p dinners.Patch) fields { + var f fields + if p.Method != nil { + f.method = *p.Method } - if row.Method.String != "grill" || row.Difficulty.String != "easy" || row.MinutesTotal.Int64 != 150 { - t.Errorf("method, difficulty, minutes = %q, %q, %d; want grill and easy kept, 150 from the recipe", - row.Method.String, row.Difficulty.String, row.MinutesTotal.Int64) + if p.MinutesTotal != nil { + f.minutes = int64(*p.MinutesTotal) } + if p.Difficulty != nil { + f.difficulty = *p.Difficulty + } + return f } // TestFixArchivedDinner: an archived dinner cannot be grounded by hand. diff --git a/internal/server/dinners.go b/internal/server/dinners.go index fcb0197..b6c627f 100644 --- a/internal/server/dinners.go +++ b/internal/server/dinners.go @@ -202,7 +202,8 @@ func actorOrFail(w http.ResponseWriter, r *http.Request, logger *slog.Logger) (a // up is 409 not_configured; a search or model failure is 502; grounding by // 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 +// 409 ground_conflict. Any other grounding failure, by candidate, link or +// manual recipe, wraps dinners.ErrGroundFailed and 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 diff --git a/internal/server/grounding_test.go b/internal/server/grounding_test.go index ed02531..e7333d9 100644 --- a/internal/server/grounding_test.go +++ b/internal/server/grounding_test.go @@ -279,6 +279,45 @@ func TestPostGroundViaRecipeURLDinnerWriteFailure(t *testing.T) { } } +// TestPostGroundUnexpectedFailures: grounding by hand that fails reading +// the dinner, or storing a manual recipe, is a 500 logged by reason and +// dinner id, never with the database error. +func TestPostGroundUnexpectedFailures(t *testing.T) { + t.Parallel() + const ( + // Renaming a column every dinner read selects fails the read. + breakRead = `ALTER TABLE dinners RENAME COLUMN cuisine TO cuisine_gone` + // The trigger's message stands in for the database error. + breakStore = `CREATE TRIGGER refuse_recipes BEFORE INSERT ON recipes BEGIN SELECT RAISE(ABORT, 'disk full'); END` + manual = `{"manual":{"ingredients":["1 lb beef"],"steps":["Brown."],"minutes":240,"method":"crockpot"}}` + ) + for _, tc := range []struct { + name, sabotage, fix, leak string + }{ + {"link read", breakRead, `{"recipe_url":"` + stewPage + `"}`, "cuisine"}, + {"manual read", breakRead, manual, "cuisine"}, + {"manual store", breakStore, manual, "disk full"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + e := newDinnersEnv(t).withRecipes(t) + id := createDinner(t, e.h, `{"name":"Sunday stew"}`) + if _, err := e.st.Writer.ExecContext(t.Context(), tc.sabotage); err != nil { + t.Fatal(err) + } + + rec, body := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground", tc.fix) + if rec.Code != http.StatusInternalServerError || errorCode(body) != "internal" { + t.Errorf("POST = %d %s, want 500 internal", rec.Code, rec.Body) + } + logs := e.logs.String() + if strings.Contains(logs, tc.leak) || !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 database error", logs) + } + }) + } +} + // TestPostGroundArchived: grounding an archived dinner by hand is a 409. func TestPostGroundArchived(t *testing.T) { t.Parallel()