Never show ideas for a failed ideas job, however it failed

A failed ideas_from_ingredients job could still show ideas through GET /api/v1/jobs/{id}: a job failed at boot as interrupted by restart keeps its last partial result, and when the write clearing the ideas itself failed the runner recorded internal_error over the uncleared result. Neither path can be fixed by a write. The jobs package gains an opt-in per-kind hook, Kind.FailedResult, which Runner.Get applies to a failed job's stored result on every read. The ideas kind registers withoutIdeas, which empties the ideas and keeps the rest of the result. Kinds that do not call FailedResult are unchanged. The write-side clear stays as a tidy-up of what is stored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-08 19:36:04 -04:00
parent 4146f31275
commit 9ed6c305cb
5 changed files with 273 additions and 58 deletions
@@ -2,6 +2,7 @@ package server

 import (
 	"context"
+	"database/sql"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -16,10 +17,12 @@ import (
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/harness"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/skills"
+	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ideas"
 	"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"
 )

 const ideasPath = "/api/v1/ideas/from-ingredients"
@@ -27,8 +30,9 @@ const ideasPath = "/api/v1/ideas/from-ingredients"
 // newIdeasRouter returns a router whose ideas service runs
 // ideas_from_ingredients through the real harness with the scripted model
 // and grounds with fake search, pages and images and the scripted
-// normalize. Its job runner is started when start is set.
-func newIdeasRouter(t *testing.T, model, normalize *llm.Fake, search *recipestest.Search, pages *recipestest.Pages, images *recipestest.Images, start bool) (http.Handler, *jobs.Runner) {
+// normalize, with its job runner and store. The runner is started when
+// start is set.
+func newIdeasRouter(t *testing.T, model, normalize *llm.Fake, search *recipestest.Search, pages *recipestest.Pages, images *recipestest.Images, start bool) (http.Handler, *jobs.Runner, *store.Store) {
 	t.Helper()
 	st := newTestStore(t)
 	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
@@ -60,22 +64,93 @@ func newIdeasRouter(t *testing.T, model, normalize *llm.Fake, search *recipestes
 		t.Fatal(err)
 	}
 	if start {
-		ctx, cancel := context.WithCancel(context.Background())
-		if err := runner.Start(ctx); err != nil {
-			cancel()
-			t.Fatal(err)
-		}
-		t.Cleanup(func() {
-			cancel()
-			runner.Wait()
-		})
+		startRunner(t, runner)
 	}
-	return h, runner
+	return h, runner, st
+}
+
+// startRunner starts runner until the test ends.
+func startRunner(t *testing.T, runner *jobs.Runner) {
+	t.Helper()
+	ctx, cancel := context.WithCancel(context.Background())
+	if err := runner.Start(ctx); err != nil {
+		cancel()
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		cancel()
+		runner.Wait()
+	})
+}
+
+// scriptIdeas scripts ideas_from_ingredients to propose names, in order;
+// a recipe grounds those in grounds, the others ground none, and search
+// fails as fail says. It returns the router's fakes.
+func scriptIdeas(t *testing.T, names, grounds []string, fail map[string]error) (*llm.Fake, *llm.Fake, *recipestest.Search, *recipestest.Pages, *recipestest.Images) {
+	t.Helper()
+	model, normalize := &llm.Fake{}, &llm.Fake{}
+	search := &recipestest.Search{Results: map[string][]recipes.Candidate{}, Fail: fail}
+	pages := &recipestest.Pages{Bodies: map[string][]byte{}}
+	images := &recipestest.Images{JPEGs: map[string][]byte{}}
+	for i, name := range grounds {
+		page, photo := fmt.Sprintf("https://r.example/%d", i), fmt.Sprintf("https://r.example/%d.jpg", i)
+		search.Results[name] = []recipes.Candidate{{URL: page}}
+		pages.Bodies[page] = recipestest.RecipePage(name, "R Kitchen", photo, 25)
+		images.JPEGs[photo] = recipestest.JPEG(t, 400, 320, uint8(i+1))
+		normalize.QueueGenerate(recipestest.NormalizedReply("stovetop", 25))
+	}
+	list := make([]string, len(names))
+	for i, name := range names {
+		list[i] = fmt.Sprintf(`{"name":%q,"uses":["rice"],"missing":[],"est_minutes":30,"method":"stovetop","difficulty":"easy","search_query":%q}`, name, name)
+	}
+	js := `{"ideas":[` + strings.Join(list, ",") + `]}`
+	model.QueueGenerate(llm.GenerateResult{Response: llm.Response{Text: js, JSON: json.RawMessage(js)}})
+	return model, normalize, search, pages, images
+}
+
+// startIdeasJob posts a request for ideas from rice and returns the job id.
+func startIdeasJob(t *testing.T, h http.Handler) string {
+	t.Helper()
+	rec, body := callDinners(t, h, http.MethodPost, ideasPath, `{"ingredients": ["rice"], "quick_only": false}`)
+	if rec.Code != http.StatusAccepted {
+		t.Fatalf("POST = %d: %s", rec.Code, rec.Body)
+	}
+	return body["jobId"].(string)
+}
+
+// assertFailedWithoutIdeas checks that GET /jobs/{id} reports job failed
+// with code and a result showing no ideas.
+func assertFailedWithoutIdeas(t *testing.T, job map[string]any, code string) {
+	t.Helper()
+	if job["status"] != "failed" || job["error"] != code {
+		t.Fatalf("GET job = %v, want failed with %s", job, code)
+	}
+	res, _ := job["result"].(map[string]any)
+	if res == nil {
+		t.Fatalf("failed job result = %v, want one showing no ideas", job["result"])
+	}
+	if list, ok := res["ideas"].([]any); !ok || len(list) != 0 {
+		t.Fatalf("failed job ideas = %v, want none", res["ideas"])
+	}
+}
+
+// storedIdeas is how many ideas job id's stored result holds.
+func storedIdeas(t *testing.T, st *store.Store, id string) int {
+	t.Helper()
+	row, err := db.New(st.Reader).GetJob(t.Context(), id)
+	if err != nil {
+		t.Fatal(err)
+	}
+	var res ideas.Result
+	if err := json.Unmarshal([]byte(row.Result.String), &res); err != nil {
+		t.Fatal(err)
+	}
+	return len(res.Ideas)
 }

 func TestPostIdeasFromIngredientsRefusesBadRequests(t *testing.T) {
 	t.Parallel()
-	h, _ := newIdeasRouter(t, &llm.Fake{}, &llm.Fake{}, &recipestest.Search{}, &recipestest.Pages{}, &recipestest.Images{}, false)
+	h, _, _ := newIdeasRouter(t, &llm.Fake{}, &llm.Fake{}, &recipestest.Search{}, &recipestest.Pages{}, &recipestest.Images{}, false)

 	for _, tc := range []struct{ body, message string }{
 		{`{"quick_only": false}`, "ingredients: required"},
@@ -108,7 +183,7 @@ func TestPostIdeasFromIngredientsRunsTheJob(t *testing.T) {
 	js := `{"ideas":[{"name":"Chicken Fried Rice","uses":["rice","chicken thighs","peas"],"missing":["2 eggs"],"est_minutes":25,"method":"stovetop","difficulty":"easy","search_query":"chicken fried rice"}]}`
 	model.QueueGenerate(llm.GenerateResult{Response: llm.Response{Text: js, JSON: json.RawMessage(js)}})
 	normalize.QueueGenerate(recipestest.NormalizedReply("stovetop", 25))
-	h, runner := newIdeasRouter(t, model, normalize, search, pages, images, true)
+	h, runner, _ := newIdeasRouter(t, model, normalize, search, pages, images, true)

 	rec, body := callDinners(t, h, http.MethodPost, ideasPath, `{"ingredients": ["Chicken thighs", "rice", "scallions"], "quick_only": true}`)
 	if rec.Code != http.StatusAccepted {
@@ -169,9 +244,6 @@ func TestPostIdeasFromIngredientsRunsTheJob(t *testing.T) {

 func TestFailedIdeasJobShowsNoIdeas(t *testing.T) {
 	t.Parallel()
-	idea := func(name string) string {
-		return fmt.Sprintf(`{"name":%q,"uses":["rice"],"missing":[],"est_minutes":30,"method":"stovetop","difficulty":"easy","search_query":%q}`, name, name)
-	}
 	for _, tc := range []struct {
 		name string
 		// grounds are the ideas a recipe grounds; the others ground none.
@@ -188,43 +260,82 @@ func TestFailedIdeasJobShowsNoIdeas(t *testing.T) {
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			model, normalize := &llm.Fake{}, &llm.Fake{}
-			search := &recipestest.Search{Results: map[string][]recipes.Candidate{}, Fail: tc.fail}
-			pages := &recipestest.Pages{Bodies: map[string][]byte{}}
-			images := &recipestest.Images{JPEGs: map[string][]byte{}}
-			for i, name := range tc.grounds {
-				page, photo := fmt.Sprintf("https://r.example/%d", i), fmt.Sprintf("https://r.example/%d.jpg", i)
-				search.Results[name] = []recipes.Candidate{{URL: page}}
-				pages.Bodies[page] = recipestest.RecipePage(name, "R Kitchen", photo, 25)
-				images.JPEGs[photo] = recipestest.JPEG(t, 400, 320, uint8(i+1))
-				normalize.QueueGenerate(recipestest.NormalizedReply("stovetop", 25))
-			}
-			list := make([]string, len(tc.ideas))
-			for i, name := range tc.ideas {
-				list[i] = idea(name)
-			}
-			js := `{"ideas":[` + strings.Join(list, ",") + `]}`
-			model.QueueGenerate(llm.GenerateResult{Response: llm.Response{Text: js, JSON: json.RawMessage(js)}})
-			h, _ := newIdeasRouter(t, model, normalize, search, pages, images, true)
-
-			rec, body := callDinners(t, h, http.MethodPost, ideasPath, `{"ingredients": ["rice"], "quick_only": false}`)
-			if rec.Code != http.StatusAccepted {
-				t.Fatalf("POST = %d: %s", rec.Code, rec.Body)
-			}
-			job := waitJob(t, h, body["jobId"].(string))
-			if job["status"] != "failed" || job["error"] != tc.code {
-				t.Fatalf("GET job = %v, want failed with %s", job, tc.code)
-			}
-			if res, _ := job["result"].(map[string]any); res == nil || len(res["ideas"].([]any)) != 0 {
-				t.Fatalf("failed job result = %v, want no ideas", job["result"])
-			}
+			model, normalize, search, pages, images := scriptIdeas(t, tc.ideas, tc.grounds, tc.fail)
+			h, _, _ := newIdeasRouter(t, model, normalize, search, pages, images, true)
+			assertFailedWithoutIdeas(t, waitJob(t, h, startIdeasJob(t, h)), tc.code)
 		})
 	}
 }

+// A job left running with ideas shown is failed at the next start as
+// interrupted, without its handler running again; it still shows no ideas.
+func TestIdeasJobInterruptedByRestartShowsNoIdeas(t *testing.T) {
+	t.Parallel()
+	h, runner, st := newIdeasRouter(t, &llm.Fake{}, &llm.Fake{}, &recipestest.Search{}, &recipestest.Pages{}, &recipestest.Images{}, false)
+	id := startIdeasJob(t, h)
+
+	// The previous process claimed the job and showed an idea, then stopped.
+	q := db.New(st.Writer)
+	now := store.Now().String()
+	if _, err := q.ClaimNextJob(t.Context(), sql.NullString{String: now, Valid: true}); err != nil {
+		t.Fatal(err)
+	}
+	partial, err := ideas.Result{
+		Stage:       ideas.StageGrounding,
+		Ingredients: []string{"rice"},
+		Ideas:       []ideas.Idea{{Name: "Fried Rice", Uses: []string{"rice"}, Missing: []string{}, Method: "stovetop", Difficulty: "easy"}},
+	}.PublicJSON()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if n, err := q.PublishJobResult(t.Context(), db.PublishJobResultParams{
+		Result: sql.NullString{String: string(partial), Valid: true}, UpdatedAt: now, ID: id,
+	}); err != nil || n != 1 {
+		t.Fatalf("publish partial result = %d, %v", n, err)
+	}
+
+	startRunner(t, runner)
+	_, job := callDinners(t, h, http.MethodGet, "/api/v1/jobs/"+id, "")
+	assertFailedWithoutIdeas(t, job, jobs.CodeInterrupted)
+	if res := job["result"].(map[string]any); res["stage"] != "grounding" {
+		t.Errorf("result = %v, want the rest of the partial result kept", res)
+	}
+	if n := storedIdeas(t, st, id); n != 1 {
+		t.Fatalf("stored ideas = %d, want the interrupted job's 1 still stored", n)
+	}
+}
+
+// When the write clearing a failed job's ideas itself fails, the job fails
+// with the runner's internal_error and its stored result keeps the ideas;
+// it still shows none.
+func TestIdeasJobWhoseClearingWriteFailsShowsNoIdeas(t *testing.T) {
+	t.Parallel()
+	model, normalize, search, pages, images := scriptIdeas(t,
+		[]string{"Fried Rice", "Mystery Stew", "Rice Bowls", "Rice Soup", "Rice Salad", "Rice Pilaf"},
+		[]string{"Fried Rice", "Rice Bowls", "Rice Soup", "Rice Salad"},
+		map[string]error{"Rice Pilaf": errors.New("boom")})
+	h, runner, st := newIdeasRouter(t, model, normalize, search, pages, images, false)
+	// Refuse any write that empties a running job's shown ideas.
+	if _, err := st.Writer.ExecContext(t.Context(), `
+		CREATE TRIGGER refuse_clearing_ideas BEFORE UPDATE OF result ON jobs
+		WHEN NEW.status = 'running'
+			AND json_array_length(OLD.result, '$.ideas') > 0
+			AND json_array_length(NEW.result, '$.ideas') = 0
+		BEGIN SELECT RAISE(ABORT, 'clearing refused'); END`); err != nil {
+		t.Fatal(err)
+	}
+	startRunner(t, runner)
+
+	id := startIdeasJob(t, h)
+	assertFailedWithoutIdeas(t, waitJob(t, h, id), jobs.CodeInternal)
+	if n := storedIdeas(t, st, id); n != 4 {
+		t.Fatalf("stored ideas = %d, want the 4 the refused write left", n)
+	}
+}
+
 func TestJobsEndpointRefusesIdeasKind(t *testing.T) {
 	t.Parallel()
-	h, _ := newIdeasRouter(t, &llm.Fake{}, &llm.Fake{}, &recipestest.Search{}, &recipestest.Pages{}, &recipestest.Images{}, false)
+	h, _, _ := newIdeasRouter(t, &llm.Fake{}, &llm.Fake{}, &recipestest.Search{}, &recipestest.Pages{}, &recipestest.Images{}, false)
 	rec, body := callDinners(t, h, http.MethodPost, "/api/v1/jobs", `{"kind": "ideas_from_ingredients", "payload": {"ingredients": [], "quick_only": false}}`)
 	if rec.Code != http.StatusBadRequest || errorCode(body) != "service_only_kind" {
 		t.Fatalf("POST /jobs = %d %v, want 400 service_only_kind", rec.Code, body)