Keep Pass history immutable and replenish atomic with decisions

Fixes the three review findings on 50399b6 (pull request #102):

- A batch now rereads every declined name inside its storing transaction,
  so a Pass committed after the batch read its input but before it stores
  can no longer let the passed dish into the deck (SQ-01).
- A decision that leaves the deck under five creates the replenish batch
  and its job in the decision's own transaction, so a decision is never
  recorded without the replenish it calls for; when they cannot be
  written, neither is the decision, and a retry decides again (SQ-02).
  StartBatch commits the batch with its job the same way, replacing the
  mutex and the fail-on-enqueue cleanup. jobs gains Kind.EnqueueTx and
  Kind.Wake for this.
- Migration 0012 gives each suggestion the name its dish was suggested
  under; declined names for batches and the household context's passed
  list read it instead of the dinner's mutable name, so renaming a passed
  dinner no longer erases it from dedupe (SQ-03).

Task: b4f2558c-227f-4627-852b-be5aa190242a

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/suggest/batch.go

@@ -196,8 +196,10 @@ func (s *Service) ground(ctx context.Context, quests []skills.SideQuest) ([]grou
 // suggested and origin side_quest, grounded in its recipe, and a pending
 // suggestion citing its Regulars by id, all in one transaction, and
 // returns the suggestion ids. A Side quest is left out when, since
-// suggest_side_quests ran, a dinner it repeats was added or a Regular it
-// cites renamed or archived.
+// suggest_side_quests ran, a dinner it repeats was added, a suggestion it
+// repeats passed, or a Regular it cites renamed or archived. Declined
+// names are read in the same transaction as the store, so a Pass committed
+// before it is never missed.
 func (s *Service) store(ctx context.Context, batchID string, quests []groundedQuest) ([]string, error) {
 	ids := []string{}
 	err := s.st.WithTx(ctx, func(tx *sql.Tx) error {
@@ -206,7 +208,10 @@ func (s *Service) store(ctx context.Context, batchID string, quests []groundedQu
 		if err != nil {
 			return fmt.Errorf("suggest: list dinners for dedupe: %w", err)
 		}
-		var names []string
+		names, err := q.ListDeclinedSuggestionNames(ctx)
+		if err != nil {
+			return fmt.Errorf("suggest: list declined names for dedupe: %w", err)
+		}
 		regulars := map[string][]string{}
 		for _, row := range existing {
 			names = append(names, row.Dinner.Name)
@@ -250,6 +255,7 @@ func (s *Service) store(ctx context.Context, batchID string, quests []groundedQu
 				ID:              id,
 				DinnerID:        d.ID,
 				BatchID:         batchID,
+				Name:            d.Name,
 				Reason:          gq.quest.Reason,
 				CitedRegularIds: string(citedJSON),
 				CreatedAt:       now,

internal/suggest/suggest.go

@@ -15,7 +15,6 @@ import (
 	"fmt"
 	"log/slog"
 	"slices"
-	"sync"
 
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/skills"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/db"
@@ -112,10 +111,6 @@ type Service struct {
 	suggest  Suggester
 	logger   *slog.Logger
 	batchJob *jobs.Kind[BatchJob]
-
-	// batchMu makes finding or creating the active batch and enqueuing its
-	// job one step, so a batch is never seen without its job.
-	batchMu sync.Mutex
 }
 
 // NewService returns a service over st that suggests with suggest, grounds
@@ -161,53 +156,54 @@ func (s *Service) StartBatch(ctx context.Context, trigger string) (Batch, error)
 	if !slices.Contains(triggers, trigger) {
 		return Batch{}, fmt.Errorf("suggest: unknown batch trigger %q", trigger)
 	}
-	s.batchMu.Lock()
-	defer s.batchMu.Unlock()
-	var row db.SuggestionBatch
-	created := false
+	var b Batch
 	err := s.st.WithTx(ctx, func(tx *sql.Tx) error {
-		q := db.New(tx)
-		cur, err := q.GetActiveSuggestionBatch(ctx)
-		if err == nil {
-			row = cur
-			return nil
-		}
-		if !errors.Is(err, sql.ErrNoRows) {
-			return fmt.Errorf("suggest: read active batch: %w", err)
-		}
-		now := store.Now().String()
-		row, err = q.CreateSuggestionBatch(ctx, db.CreateSuggestionBatchParams{
-			ID:        store.NewID(),
-			Trigger:   trigger,
-			CreatedAt: now,
-			UpdatedAt: now,
-		})
-		if err != nil {
-			return fmt.Errorf("suggest: create batch: %w", err)
-		}
-		created = true
-		return nil
+		var err error
+		b, err = s.startBatch(ctx, tx, trigger)
+		return err
 	})
 	if err != nil {
 		return Batch{}, err
 	}
-	b := Batch{ID: row.ID, Trigger: row.Trigger, Status: row.Status, Created: created}
-	if !created {
-		b.JobID, err = db.New(s.st.Reader).GetSuggestionBatchJobID(ctx, db.GetSuggestionBatchJobIDParams{Kind: KindSuggestionBatch, BatchID: row.ID})
-		if err != nil {
-			return Batch{}, fmt.Errorf("suggest: read job of batch %s: %w", row.ID, err)
-		}
-		return b, nil
-	}
-	b.JobID, err = s.batchJob.Enqueue(ctx, BatchJob{BatchID: row.ID})
-	if err != nil {
-		// The batch has no job to run it, so it must not block the next.
-		s.finishBatch(context.WithoutCancel(ctx), row.ID, BatchFailed)
-		return Batch{}, fmt.Errorf("suggest: enqueue batch %s: %w", row.ID, err)
+	if b.Created {
+		s.batchJob.Wake()
 	}
 	return b, nil
 }
 
+// startBatch does StartBatch's work in tx: the batch and the job that runs
+// it are committed together, so a batch is never seen without its job. The
+// caller wakes the job runner once tx has committed and Created is true.
+func (s *Service) startBatch(ctx context.Context, tx *sql.Tx, trigger string) (Batch, error) {
+	q := db.New(tx)
+	cur, err := q.GetActiveSuggestionBatch(ctx)
+	if err == nil {
+		jobID, err := q.GetSuggestionBatchJobID(ctx, db.GetSuggestionBatchJobIDParams{Kind: KindSuggestionBatch, BatchID: cur.ID})
+		if err != nil {
+			return Batch{}, fmt.Errorf("suggest: read job of batch %s: %w", cur.ID, err)
+		}
+		return Batch{ID: cur.ID, JobID: jobID, Trigger: cur.Trigger, Status: cur.Status}, nil
+	}
+	if !errors.Is(err, sql.ErrNoRows) {
+		return Batch{}, fmt.Errorf("suggest: read active batch: %w", err)
+	}
+	now := store.Now().String()
+	row, err := q.CreateSuggestionBatch(ctx, db.CreateSuggestionBatchParams{
+		ID:        store.NewID(),
+		Trigger:   trigger,
+		CreatedAt: now,
+		UpdatedAt: now,
+	})
+	if err != nil {
+		return Batch{}, fmt.Errorf("suggest: create batch: %w", err)
+	}
+	jobID, err := s.batchJob.EnqueueTx(ctx, tx, BatchJob{BatchID: row.ID})
+	if err != nil {
+		return Batch{}, fmt.Errorf("suggest: enqueue batch %s: %w", row.ID, err)
+	}
+	return Batch{ID: row.ID, JobID: jobID, Trigger: row.Trigger, Status: row.Status, Created: true}, nil
+}
+
 // finishBatch records batch id as status, logging a failure to do so.
 func (s *Service) finishBatch(ctx context.Context, id, status string) {
 	if _, err := db.New(s.st.Writer).FinishSuggestionBatch(ctx, db.FinishSuggestionBatchParams{
@@ -289,9 +285,9 @@ func (s *Service) Accept(ctx context.Context, id, updatedBy string) (Decision, e
 }
 
 // Decline records Pass on suggestion id, archiving its dinner and
-// recording updatedBy on it. The archived dinner keeps its name, and the
-// declined suggestion is read into every later batch, so the dish is never
-// suggested again.
+// recording updatedBy on it. The suggestion keeps the name its dish was
+// suggested under, whatever the dinner is later renamed, and every later
+// batch reads it as declined, so the dish is never suggested again.
 func (s *Service) Decline(ctx context.Context, id, updatedBy string) (Decision, error) {
 	return s.decide(ctx, id, StatusDeclined, dinners.StatusArchived, updatedBy)
 }
@@ -300,15 +296,15 @@ func (s *Service) Decline(ctx context.Context, id, updatedBy string) (Decision,
 // dinnerStatus. Repeating a decision changes nothing and returns the
 // suggestion as it is; the opposite decision is a *DecidedError. A
 // decision that leaves the deck under ReplenishBelow starts a replenish
-// batch unless one is already pending or running. The decision is
-// committed before the batch starts, so a failure to start it is returned
-// with the decision already made.
+// batch unless one is already pending or running, in the decision's own
+// transaction: the decision is never recorded without the replenish it
+// calls for, and a failure to start the batch records neither.
 func (s *Service) decide(ctx context.Context, id, decision, dinnerStatus, updatedBy string) (Decision, error) {
 	if updatedBy == "" {
 		return Decision{}, errors.New("suggest: decide: no member to record as updated_by")
 	}
 	var row db.GetSuggestionRow
-	changed := false
+	var replenish *Batch
 	err := s.st.WithTx(ctx, func(tx *sql.Tx) error {
 		q := db.New(tx)
 		cur, err := q.GetSuggestion(ctx, id)
@@ -349,33 +345,34 @@ func (s *Service) decide(ctx context.Context, id, decision, dinnerStatus, update
 		if err != nil {
 			return fmt.Errorf("suggest: read suggestion %s: %w", id, err)
 		}
-		changed = true
+		left, err := q.CountDeck(ctx)
+		if err != nil {
+			return fmt.Errorf("suggest: count deck: %w", err)
+		}
+		if left >= ReplenishBelow {
+			return nil
+		}
+		b, err := s.startBatch(ctx, tx, TriggerReplenish)
+		if err != nil {
+			return err
+		}
+		replenish = &b
 		return nil
 	})
 	if err != nil {
 		return Decision{}, err
 	}
+	created := replenish != nil && replenish.Created
+	if created {
+		s.batchJob.Wake()
+	}
 	sug, err := fromRow(row.Suggestion, row.Dinner)
 	if err != nil {
 		return Decision{}, err
 	}
 	d := Decision{Suggestion: sug}
-	if !changed {
-		return d, nil
-	}
-	left, err := db.New(s.st.Reader).CountDeck(ctx)
-	if err != nil {
-		return Decision{}, fmt.Errorf("suggest: count deck: %w", err)
-	}
-	if left >= ReplenishBelow {
-		return d, nil
-	}
-	b, err := s.StartBatch(ctx, TriggerReplenish)
-	if err != nil {
-		return Decision{}, err
-	}
-	if b.Created {
-		d.ReplenishJobID = &b.JobID
+	if created {
+		d.ReplenishJobID = &replenish.JobID
 	}
 	return d, nil
 }

internal/suggest/suggest_test.go

@@ -17,6 +17,7 @@ import (
 	"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/dinners"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/household"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/jobs"
 	"code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/recipes"
@@ -41,9 +42,12 @@ type env struct {
 	side  *llm.Fake
 	// regulars are the Regulars' ids by name.
 	regulars map[string]string
+	grounder *recipes.Grounder
 
 	mu     sync.Mutex
 	inputs []skills.SideQuestsRequest
+	// stub, when set, answers in place of suggest_side_quests.
+	stub func(in skills.SideQuestsRequest) skills.SideQuests
 }
 
 // newEnv returns a suggestions service over a migrated store with the
@@ -83,6 +87,7 @@ func newEnv(t *testing.T) *env {
 		e.regulars[name] = e.dinner(t, name, "regular")
 	}
 	g := recipestest.NewGrounder(t, st, dataDir, e.search, e.pages, e.images, e.model)
+	e.grounder = g
 	guard, err := harness.NewGuard(harness.DefaultMaxCalls)
 	if err != nil {
 		t.Fatal(err)
@@ -96,7 +101,11 @@ func newEnv(t *testing.T) *env {
 	suggester := func(ctx context.Context, scope string, in skills.SideQuestsRequest) (skills.SideQuests, error) {
 		e.mu.Lock()
 		e.inputs = append(e.inputs, in)
+		stub := e.stub
 		e.mu.Unlock()
+		if stub != nil {
+			return stub(in), nil
+		}
 		return skills.SuggestSideQuests(ctx, cfg, scope, in)
 	}
 	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
@@ -169,6 +178,7 @@ func (e *env) seedDeck(t *testing.T, names ...string) []string {
 			ID:              id,
 			DinnerID:        e.dinner(t, name, "suggested"),
 			BatchID:         b.ID,
+			Name:            name,
 			Reason:          "Like your Beef Chili.",
 			CitedRegularIds: fmt.Sprintf("[%q]", e.regulars["Beef Chili"]),
 			CreatedAt:       store.Now().String(),
@@ -672,6 +682,146 @@ func TestLaterBatchNeverResuggestsDeclined(t *testing.T) {
 	}
 }
 
+// TestBatchNeverStoresDishPassedWhileRunning passes a Side quest after a
+// batch has read its input but before it stores, with suggest_side_quests
+// answering from that input: the batch must not store the passed dish.
+func TestBatchNeverStoresDishPassedWhileRunning(t *testing.T) {
+	e := newEnv(t)
+	e.start(t)
+	ids := e.seedDeck(t, "Chicken Shawarma Bowls")
+	e.recipe(t, "Chicken Shawarma Bowls")
+	e.recipe(t, "Beef Ragu")
+	held, release := make(chan struct{}), make(chan struct{})
+	stub := func(skills.SideQuestsRequest) skills.SideQuests {
+		close(held)
+		<-release
+		quest := func(name, cited string) skills.SideQuest {
+			return skills.SideQuest{Name: name, Cuisine: "Fusion", Method: "stovetop", EstMinutes: 35, Difficulty: "easy",
+				Reason: "Like your " + cited + ".", CitedRegulars: []string{cited}, SearchQuery: name}
+		}
+		return skills.SideQuests{Suggestions: []skills.SideQuest{
+			quest("Chicken Shawarma Bowls", "Chicken Fajitas"),
+			quest("Beef Ragu", "Beef Chili"),
+		}}
+	}
+	e.mu.Lock()
+	e.stub = stub
+	e.mu.Unlock()
+
+	b, err := e.svc.StartBatch(t.Context(), suggest.TriggerManual)
+	if err != nil {
+		t.Fatal(err)
+	}
+	<-held
+	if _, err := e.svc.Decline(t.Context(), ids[0], member); err != nil {
+		close(release)
+		t.Fatal(err)
+	}
+	close(release)
+	job := e.waitJob(t, b.JobID)
+	if job.Status != jobs.StatusSucceeded {
+		t.Fatalf("batch job %s: %v", job.Status, *job.Error)
+	}
+	var res batchResult
+	if err := json.Unmarshal(job.Result, &res); err != nil {
+		t.Fatal(err)
+	}
+	if len(res.SuggestionIDs) != 1 || res.Dropped != 1 {
+		t.Errorf("batch result = %+v, want 1 kept and the passed dish dropped", res)
+	}
+	if got := e.deckNames(t); !slices.Equal(got, []string{"Beef Ragu"}) {
+		t.Errorf("deck = %v, want [Beef Ragu]", got)
+	}
+}
+
+// TestLaterBatchNeverResuggestsRenamedPass passes a Side quest, renames
+// its archived dinner, and checks the next batch still refuses the name it
+// was suggested under.
+func TestLaterBatchNeverResuggestsRenamedPass(t *testing.T) {
+	e := newEnv(t) // runner not started: the replenish batch waits for the rename
+	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
+	dinnerSvc, err := dinners.NewService(e.st, e.grounder, e.runner, logger)
+	if err != nil {
+		t.Fatal(err)
+	}
+	ids := e.seedDeck(t, "Chicken Shawarma Bowls")
+	d, err := e.svc.Decline(t.Context(), ids[0], member)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if d.ReplenishJobID == nil {
+		t.Fatal("passing the last Side quest started no replenish batch")
+	}
+	renamed := "Leftover Wrap"
+	if _, err := dinnerSvc.Update(t.Context(), d.Suggestion.DinnerID, dinners.Patch{Name: &renamed}, member); err != nil {
+		t.Fatal(err)
+	}
+
+	// The respelled passed dish would ground, so only the declined names
+	// keep it out of the deck.
+	e.recipe(t, "chicken shawarma bowls!")
+	e.recipe(t, "Beef Ragu")
+	e.reply(
+		sideQuest("chicken shawarma bowls!", "Spiced chicken, like your Chicken Fajitas.", "Chicken Fajitas"),
+		sideQuest("Beef Ragu", "Slow beef like your Beef Chili.", "Beef Chili"),
+	)
+	e.start(t)
+	job := e.waitJob(t, *d.ReplenishJobID)
+	if job.Status != jobs.StatusSucceeded {
+		t.Fatalf("replenish job %s: %v", job.Status, *job.Error)
+	}
+	if got := e.deckNames(t); !slices.Equal(got, []string{"Beef Ragu"}) {
+		t.Errorf("deck = %v, want [Beef Ragu]", got)
+	}
+	e.mu.Lock()
+	in := e.inputs[0]
+	e.mu.Unlock()
+	if !slices.Equal(in.Declined, []string{"Chicken Shawarma Bowls"}) || !slices.Equal(in.Context.Passed, []string{"Chicken Shawarma Bowls"}) {
+		t.Errorf("replenish batch declined %v, passed %v; want the name it was suggested under in both", in.Declined, in.Context.Passed)
+	}
+}
+
+// TestDecisionNeverCommitsWithoutItsReplenish makes the replenish batch's
+// job fail to store: the decision must not be recorded either, so the
+// retry decides again and starts the replenish.
+func TestDecisionNeverCommitsWithoutItsReplenish(t *testing.T) {
+	e := newEnv(t) // runner not started: the replenish batch stays pending
+	ids := e.seedDeck(t, "Pho", "Ramen", "Laksa", "Bibimbap", "Katsu")
+	if _, err := e.st.Writer.ExecContext(t.Context(), `CREATE TRIGGER refuse_jobs BEFORE INSERT ON jobs BEGIN SELECT RAISE(ABORT, 'jobs refused'); END`); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := e.svc.Decline(t.Context(), ids[0], member); err == nil {
+		t.Fatal("decline succeeded with no replenish job stored")
+	}
+	if got := e.deckNames(t); len(got) != 5 {
+		t.Errorf("deck after the failed decline = %v, want all 5 still pending", got)
+	}
+	if n := e.activeBatches(t); n != 0 {
+		t.Errorf("%d active batches after the failed decline, want 0", n)
+	}
+
+	if _, err := e.st.Writer.ExecContext(t.Context(), `DROP TRIGGER refuse_jobs`); err != nil {
+		t.Fatal(err)
+	}
+	d, err := e.svc.Decline(t.Context(), ids[0], member)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if d.Suggestion.Status != suggest.StatusDeclined || d.ReplenishJobID == nil {
+		t.Fatalf("retried decline = %+v, want declined with a replenish job", d)
+	}
+	job, err := e.runner.Get(t.Context(), *d.ReplenishJobID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if job.Kind != suggest.KindSuggestionBatch || job.Status != jobs.StatusQueued {
+		t.Errorf("replenish job = %+v, want a queued %s", job, suggest.KindSuggestionBatch)
+	}
+	if n := e.activeBatches(t); n != 1 {
+		t.Errorf("%d active batches, want 1", n)
+	}
+}
+
 func TestBatchFailsWhenAIFails(t *testing.T) {
 	e := newEnv(t)
 	e.start(t)