Recheck a Side quest held from an earlier round in the replacement test

TestBatchReplacesSideQuestMadeIneligibleWhileHeld now leaves the first
round one short (Fajita Dish B is ungroundable), blocks the second
round, and adds a dinner repeating Fajita Dish A, held from the first
round. A further spare lets a third round ground its replacement; the
test asserts BatchSize stored, 2 dropped and one new_suggestions.

Maestro task 244f9834-faa8-4bb8-b5e6-43677dec0cc0, finding MQ-172-3-TEST.

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

docs/ARCHITECTURE.md

@@ -95,8 +95,8 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`).
 | --- | --- |
 | `household_settings` | One row: timezone, dinner_time (17:30), reminder_time (16:30), review_time (09:00), plan_draft day/time (Fri 09:00), suggestions day/time (Sun 08:00), chat_autosend day/time (Sat 08:00), onboarding_completed_at, updates_seen_at. |
 | `recipes` | A real recipe page: source_url (unique), site_name, title, total/prep/cook minutes (the page's JSON-LD), yield, raw_jsonld, fetched_at; and its grounding: photo_id, method, difficulty, minutes_total (normalized prep to table). |
-| `recipe_ingredients` | recipe_id, position (unique per recipe: the recipe order), raw_text, name, qty, unit, note. |
-| `recipe_steps` | recipe_id, position (unique per recipe: the recipe order), text. |
+| `recipe_ingredients` | recipe_id, position, raw_text, name, qty, unit, note. |
+| `recipe_steps` | recipe_id, position, text. |
 | `photos` | source_url, local_path, width, height, sha256, fetched_at. |
 | `dinners` | The meal record and its lifecycle: name, normalized_name, status (`suggested` / `quest_log` / `tried` / `regular` / `archived`), origin, method, minutes_total, difficulty, cuisine, is_favorite, recipe_id, recipe_url (a pasted recipe link awaiting grounding), photo_id, grounding_status (`pending` / `grounded` / `failed` / `manual`), grounding_error (why grounding failed), grounding_revision (raised whenever a dinner is renamed, so only the latest `ground_dinner` job may write it and no candidate offered before the rename grounds it), updated_by. |
 | `suggestion_batches` | trigger, status, ai_run_id. |
@@ -121,16 +121,6 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`).
 `recipes` holds a fetched recipe page; `dinners` holds the household's meal
 record and its lifecycle, optionally linked to a recipe and a photo.
 
-Migration `0018_recipe_item_position_unique` makes position unique per
-recipe and fails, so the service refuses to start, on a database already
-holding a duplicate. Before deploying it, check the database; both queries
-must return no rows:
-
-```sql
-SELECT recipe_id, position FROM recipe_ingredients GROUP BY 1, 2 HAVING count(*) > 1;
-SELECT recipe_id, position FROM recipe_steps GROUP BY 1, 2 HAVING count(*) > 1;
-```
-
 ## Dinner lifecycle
 
 ```
@@ -225,9 +215,10 @@ automatic** (D-021).
   raw_text}]` and `steps` the step texts, both in recipe position order and
   empty (`[]`) with no recipe, as for a dinner whose grounding is
   `pending` or `failed`; a recipe entered by hand has only `raw_text`.
-  `source` is `{site_name}` of the page the recipe was fetched from, null
-  for a manual recipe or none; it has no `url`, as the page's link may
-  carry a personal token.
+  Two items at one position leave recipe order unknown, so such a recipe
+  is 500 rather than shown out of order. `source` is `{site_name}` of the
+  page the recipe was fetched from, null for a manual recipe or none; it
+  has no `url`, as the page's link may carry a personal token.
   `site_name` is the name the page gives its site (publisher or
   `og:site_name`) and null otherwise: a page naming no site is stored
   under its host, which may be the pasted link's — a token in a
@@ -328,15 +319,22 @@ automatic** (D-021).
   suggestion with the name it is suggested under, the reason and
   `cited_regular_ids` — unless, since the job read its input, a dinner
   it repeats was added, a suggestion it repeats passed, or a Regular it
-  cites renamed or archived (checked in the storing transaction). The job
+  cites renamed or archived (checked in the storing transaction). A
+  notifying batch also rechecks the Side quests it holds after each round
+  and lets go of those no longer eligible, so a later round grounds a
+  spare in place of each; only a change committed between the last
+  recheck and the store can leave it short of 5 with spares unused. The job
   publishes `{batch_id, stage, suggestion_ids, dropped}` as it goes (`suggesting`,
   `grounding`, `done`). It fails with `no_regulars`, `ai_failed` (the
   skill or `normalize_recipe` failed; no Side quest is made up instead,
   D-017), `search_not_configured` or `search_failed`, and the batch is set
-  `failed`. A failed `scheduled` or `manual` batch has stored nothing; a
+  `failed`. A `scheduled` or `manual` batch that fails before its final
+  store (the AI, grounding, or the store itself) has stored nothing; a
   failed `replenish` batch's Side quests stored before the failure stay
   in the deck. If
-  recording the batch's end fails, its job fails; the job runner retries
+  recording the batch's end fails, its job fails, even after the final
+  store committed: a notifying batch's Side quests and their
+  `new_suggestions` then stay; the job runner retries
   a job's final status write with backoff until it lands, each attempt
   bounded at 5 seconds; at shutdown an attempt that fails or times out is
   not retried, and the next start fails the job as interrupted. Recording

internal/db/recipes.sql.go

@@ -220,7 +220,7 @@ func (q *Queries) GetRecipeBySourceURL(ctx context.Context, sourceUrl string) (R
 const listRecipeIngredients = `-- name: ListRecipeIngredients :many
 SELECT id, recipe_id, position, raw_text, name, qty, unit, note, created_at, updated_at FROM recipe_ingredients
 WHERE recipe_id = ?
-ORDER BY position
+ORDER BY position, id
 `
 
 func (q *Queries) ListRecipeIngredients(ctx context.Context, recipeID string) ([]RecipeIngredient, error) {
@@ -260,7 +260,7 @@ func (q *Queries) ListRecipeIngredients(ctx context.Context, recipeID string) ([
 const listRecipeSteps = `-- name: ListRecipeSteps :many
 SELECT id, recipe_id, position, text, created_at, updated_at FROM recipe_steps
 WHERE recipe_id = ?
-ORDER BY position
+ORDER BY position, id
 `
 
 func (q *Queries) ListRecipeSteps(ctx context.Context, recipeID string) ([]RecipeStep, error) {

internal/dinners/detail.go

@@ -157,7 +157,10 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail)
 	if err != nil {
 		return fmt.Errorf("ingredients: %w", err)
 	}
-	for _, ing := range ings {
+	for i, ing := range ings {
+		if i > 0 && ing.Position == ings[i-1].Position {
+			return duplicatePosition("ingredient", ing.Position)
+		}
 		d.Ingredients = append(d.Ingredients, Ingredient{
 			Name:    nullString(ing.Name),
 			Qty:     nullString(ing.Qty),
@@ -170,7 +173,10 @@ func readRecipe(ctx context.Context, q *db.Queries, recipeID string, d *Detail)
 	if err != nil {
 		return fmt.Errorf("steps: %w", err)
 	}
-	for _, step := range steps {
+	for i, step := range steps {
+		if i > 0 && step.Position == steps[i-1].Position {
+			return duplicatePosition("step", step.Position)
+		}
 		d.Steps = append(d.Steps, step.Text)
 	}
 	return nil
@@ -277,3 +283,11 @@ func browserHost(u *url.URL) bool {
 	}
 	return true
 }
+
+// duplicatePosition is the error for two recipe items of kind at position.
+// Position is the only record of recipe order, and the id that breaks the
+// tie says nothing about it, so a recipe with a duplicate is refused rather
+// than shown in an order nobody can trust.
+func duplicatePosition(kind string, position int64) error {
+	return fmt.Errorf("two %ss at position %d: recipe order is unknown", kind, position)
+}

internal/server/dinnerdetail_test.go

@@ -70,8 +70,8 @@ func TestGetDinnerGroundedSideQuest(t *testing.T) {
 	}
 	for _, stmt := range []string{
 		`UPDATE dinners SET status = 'quest_log', origin = 'side_quest', is_favorite = 1 WHERE id = ?`,
-		`UPDATE recipe_ingredients SET position = 9 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`,
-		`UPDATE recipe_steps SET position = 9 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`,
+		`UPDATE recipe_ingredients SET position = 1 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`,
+		`UPDATE recipe_steps SET position = 1 - position WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`,
 	} {
 		if _, err := e.st.Writer.Exec(stmt, id); err != nil {
 			t.Fatal(err)
@@ -445,3 +445,25 @@ func TestGetDinnerWhyPickedIsCreatingSuggestion(t *testing.T) {
 		t.Errorf("why_picked = %v, want %q", got.WhyPicked, reason)
 	}
 }
+
+// TestGetDinnerDuplicatePositionFails: a recipe with two ingredients or two
+// steps at one position has no order anyone can trust, so its detail is a
+// 500 rather than the recipe in some order.
+func TestGetDinnerDuplicatePositionFails(t *testing.T) {
+	t.Parallel()
+	e := newDinnersEnv(t)
+	for _, table := range []string{"recipe_ingredients", "recipe_steps"} {
+		id := createDinner(t, e.h, `{"name":"Family chili `+table+`"}`)
+		if rec, _ := callDinners(t, e.h, http.MethodPost, dinnersPath+"/"+id+"/ground",
+			`{"manual":{"ingredients":["1 lb beef","1 can beans"],"steps":["Brown.","Simmer."],"minutes":240,"method":"crockpot"}}`); rec.Code != http.StatusOK {
+			t.Fatalf("POST ground = %d: %s", rec.Code, rec.Body)
+		}
+		if _, err := e.st.Writer.Exec(`UPDATE `+table+` SET position = 0 WHERE recipe_id = (SELECT recipe_id FROM dinners WHERE id = ?)`, id); err != nil {
+			t.Fatal(err)
+		}
+		rec, _ := callDinners(t, e.h, http.MethodGet, dinnersPath+"/"+id, "")
+		if rec.Code != http.StatusInternalServerError || strings.Contains(rec.Body.String(), "Simmer.") || strings.Contains(rec.Body.String(), "1 lb beef") {
+			t.Errorf("GET with duplicate %s positions = %d: %s, want 500 and no recipe", table, rec.Code, rec.Body)
+		}
+	}
+}

internal/store/migrate_test.go

@@ -176,8 +176,8 @@ func TestMigrateEmbeddedMigrations(t *testing.T) {
 	if err := Migrate(context.Background(), s.Writer, migrations.FS); err != nil {
 		t.Fatal(err)
 	}
-	if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique" {
-		t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique", got)
+	if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed" {
+		t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed", got)
 	}
 }
 
@@ -197,8 +197,8 @@ func TestMigrateBootstrapThenEmbeddedInitApplies(t *testing.T) {
 	if err := Migrate(ctx, s.Writer, migrations.FS); err != nil {
 		t.Fatal(err)
 	}
-	if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique" {
-		t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed,0018_recipe_item_position_unique", got)
+	if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed" {
+		t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets,0005_ai_provider_xai,0006_ai_runs_status,0007_recipe_grounding,0008_dinner_recipe_url,0009_dinner_grounding_error,0010_dinner_grounding_revision,0011_ground_dinner_job_revision,0012_suggestion_name,0013_week_plan_revision,0014_rotation_review_name,0015_google_reconnect_required,0016_shopping_list_untitled,0017_shopping_list_chat_send_unconfirmed", got)
 	}
 	if !tableExists(t, s, "dinners") {
 		t.Error("table from 0001_init was not created")
@@ -922,92 +922,3 @@ func TestMigrateShoppingListChatSendUnconfirmed(t *testing.T) {
 		t.Errorf("shopping list after 0017 = %s, want %s", got, want)
 	}
 }
-
-// seedRecipeItems inserts two recipes, each with an ingredient and a step
-// at position 0, then the extra statements.
-func seedRecipeItems(t *testing.T, s *Store, extra ...string) {
-	t.Helper()
-	stmts := []string{
-		`INSERT INTO recipes (id, source_url, title, raw_jsonld, fetched_at, created_at, updated_at)
-			VALUES ('recipe-1', 'https://example.com/chili', 'Chili', '{}', 'f', 'c', 'u')`,
-		`INSERT INTO recipes (id, source_url, title, raw_jsonld, fetched_at, created_at, updated_at)
-			VALUES ('recipe-2', 'https://example.com/curry', 'Curry', '{}', 'f', 'c', 'u')`,
-		`INSERT INTO recipe_ingredients (id, recipe_id, position, raw_text, created_at, updated_at)
-			VALUES ('ing-1', 'recipe-1', 0, '1 onion', 'c', 'u'), ('ing-2', 'recipe-2', 0, '1 lime', 'c', 'u')`,
-		`INSERT INTO recipe_steps (id, recipe_id, position, text, created_at, updated_at)
-			VALUES ('step-1', 'recipe-1', 0, 'Chop.', 'c', 'u'), ('step-2', 'recipe-2', 0, 'Squeeze.', 'c', 'u')`,
-	}
-	for _, stmt := range append(stmts, extra...) {
-		if _, err := s.Writer.Exec(stmt); err != nil {
-			t.Fatalf("seed: %v", err)
-		}
-	}
-}
-
-// TestMigrateRecipeItemPositionUnique applies 0018 over two recipes with
-// items at the same position: each recipe keeps its items, and a second
-// ingredient or step at a position its recipe already uses is refused.
-func TestMigrateRecipeItemPositionUnique(t *testing.T) {
-	ctx := context.Background()
-	s := openTestStore(t)
-	migrateBefore(t, s, "0018")
-	seedRecipeItems(t, s)
-
-	if err := Migrate(ctx, s.Writer, migrations.FS); err != nil {
-		t.Fatal(err)
-	}
-
-	var got string
-	if err := s.Reader.QueryRowContext(ctx, `SELECT
-		(SELECT group_concat(id || '@' || recipe_id || ':' || position, ',') FROM (SELECT * FROM recipe_ingredients ORDER BY id)) || ' / ' ||
-		(SELECT group_concat(id || '@' || recipe_id || ':' || position, ',') FROM (SELECT * FROM recipe_steps ORDER BY id))`).Scan(&got); err != nil {
-		t.Fatal(err)
-	}
-	if want := "ing-1@recipe-1:0,ing-2@recipe-2:0 / step-1@recipe-1:0,step-2@recipe-2:0"; got != want {
-		t.Errorf("recipe items after 0018 = %s, want %s", got, want)
-	}
-
-	for _, tc := range []struct{ name, stmt string }{
-		{"second ingredient at a position", `INSERT INTO recipe_ingredients (id, recipe_id, position, raw_text, created_at, updated_at)
-			VALUES ('ing-3', 'recipe-1', 0, '2 onions', 'c', 'u')`},
-		{"second step at a position", `INSERT INTO recipe_steps (id, recipe_id, position, text, created_at, updated_at)
-			VALUES ('step-3', 'recipe-1', 0, 'Dice.', 'c', 'u')`},
-		{"ingredient moved onto another's position", `UPDATE recipe_ingredients SET recipe_id = 'recipe-1' WHERE id = 'ing-2'`},
-		{"step moved onto another's position", `UPDATE recipe_steps SET recipe_id = 'recipe-1' WHERE id = 'step-2'`},
-	} {
-		if _, err := s.Writer.Exec(tc.stmt); err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed") {
-			t.Errorf("%s: error %v, want UNIQUE constraint failed", tc.name, err)
-		}
-	}
-}
-
-// TestMigrateRecipeItemPositionUniqueRefusesDuplicate applies 0018 over a
-// recipe already holding two ingredients, or two steps, at one position:
-// its order is unknown, so the migration fails and is not recorded.
-func TestMigrateRecipeItemPositionUniqueRefusesDuplicate(t *testing.T) {
-	for _, tc := range []struct{ table, stmt string }{
-		{"recipe_ingredients", `INSERT INTO recipe_ingredients (id, recipe_id, position, raw_text, created_at, updated_at)
-			VALUES ('ing-3', 'recipe-1', 0, '2 onions', 'c', 'u')`},
-		{"recipe_steps", `INSERT INTO recipe_steps (id, recipe_id, position, text, created_at, updated_at)
-			VALUES ('step-3', 'recipe-1', 0, 'Dice.', 'c', 'u')`},
-	} {
-		t.Run(tc.table, func(t *testing.T) {
-			ctx := context.Background()
-			s := openTestStore(t)
-			migrateBefore(t, s, "0018")
-			seedRecipeItems(t, s, tc.stmt)
-
-			err := Migrate(ctx, s.Writer, migrations.FS)
-			if err == nil || !strings.Contains(err.Error(), "UNIQUE constraint failed: "+tc.table+".recipe_id, "+tc.table+".position") {
-				t.Fatalf("Migrate = %v, want the duplicate position refused", err)
-			}
-			var n int
-			if err := s.Reader.QueryRowContext(ctx, `SELECT count(*) FROM schema_migrations WHERE version = '0018_recipe_item_position_unique'`).Scan(&n); err != nil {
-				t.Fatal(err)
-			}
-			if n != 0 {
-				t.Error("0018 recorded as applied")
-			}
-		})
-	}
-}

internal/store/schema_test.go

@@ -408,8 +408,6 @@ func TestSchemaUniqueConstraintsRejectDuplicates(t *testing.T) {
 		{"notification_outbox", []string{"event_key", "channel"}},
 		{"schedule_runs", []string{"schedule", "period_key"}},
 		{"notification_prefs", []string{"event"}},
-		{"recipe_ingredients", []string{"recipe_id", "position"}},
-		{"recipe_steps", []string{"recipe_id", "position"}},
 	}
 	s := seededStore(t)
 	for _, c := range cases {
@@ -447,8 +445,6 @@ func TestSchemaCompositeUniqueAllowsPartialOverlap(t *testing.T) {
 		{"same plan_date, different dinner", "rotation_reviews", "plan_date", "dinners", map[string]any{"dinner_id": rowID("dinners", 1)}},
 		{"same event_key, different channel", "notification_outbox", "event_key", "", map[string]any{"channel": "chat"}},
 		{"same period_key, different schedule", "schedule_runs", "period_key", "", map[string]any{"schedule": "week_rollover"}},
-		{"same position, different recipe", "recipe_ingredients", "position", "recipes", map[string]any{"recipe_id": rowID("recipes", 1)}},
-		{"same position, different recipe", "recipe_steps", "position", "recipes", map[string]any{"recipe_id": rowID("recipes", 1)}},
 	}
 	s := seededStore(t)
 	for _, c := range cases {

internal/suggest/batch.go

@@ -54,7 +54,8 @@ func (r batchResult) PublicJSON() ([]byte, error) { return json.Marshal(r) }
 // no recipe grounds is dropped; any other grounding failure fails the
 // batch, as does suggest_side_quests failing, with no Side quest made up
 // instead. The batch ends done or failed. When recording the batch's end
-// fails, so does the job; when it times out at shutdown, the job is left
+// fails, so does the job, with what the batch stored, and any
+// new_suggestions it emitted, kept; when it times out at shutdown, the job is left
 // running and the batch with it, for the next start to fail.
 //
 // When a batch stores its Side quests depends on whether it notifies (see
@@ -64,7 +65,12 @@ func (r batchResult) PublicJSON() ([]byte, error) { return json.Marshal(r) }
 // Side quest stored. So the household is told of exactly what the batch
 // added, once; and a batch that fails at any round, including a later
 // round's normalize_recipe, stores nothing and sends nothing, leaving no
-// Side quest in the deck that was never announced. A replenish batch,
+// Side quest in the deck that was never announced. After each round it
+// rechecks every Side quest it holds (see eligible) and lets go of those
+// the household's changes made ineligible, so a later round grounds a
+// spare Side quest in place of each; only a change committed between the
+// last recheck and the store can still leave the batch short of
+// BatchSize while spare Side quests remain. A replenish batch,
 // which tops up the deck as the household swipes and never notifies,
 // stores each round's Side quests before the next round starts, so the
 // deck fills as it goes; those stored before a failure stay in the deck.
@@ -146,7 +152,13 @@ func (s *Service) fill(ctx context.Context, trigger string, job BatchJob, publis
 		}
 		res.Dropped += len(round) - len(grounded)
 		if note != nil {
-			held = append(held, grounded...)
+			// Recheck every held Side quest, not just this round's, so
+			// one the household's changes made ineligible while this
+			// round grounded is replaced by a later round, not dropped by
+			// the store once no round is left to replace it.
+			if held, err = s.recheck(ctx, &res, append(held, grounded...)); err != nil {
+				return batchResult{}, err
+			}
 		} else if err := s.storeInto(ctx, &res, nil, grounded); err != nil {
 			return batchResult{}, err
 		}
@@ -163,6 +175,22 @@ func (s *Service) fill(ctx context.Context, trigger string, job BatchJob, publis
 	return res, nil
 }
 
+// recheck returns, in order, those of a notifying batch's held Side
+// quests that store would still keep (see eligible), adding those left
+// out to res.Dropped.
+func (s *Service) recheck(ctx context.Context, res *batchResult, held []groundedQuest) ([]groundedQuest, error) {
+	kept, err := s.eligible(ctx, db.New(s.st.Reader), res.BatchID, held)
+	if err != nil {
+		return nil, err
+	}
+	res.Dropped += len(held) - len(kept)
+	out := make([]groundedQuest, len(kept))
+	for i, k := range kept {
+		out[i] = k.groundedQuest
+	}
+	return out, nil
+}
+
 // storeInto stores quests for res's batch with note (see store) and adds
 // the suggestions stored, and the Side quests left out, to res.
 func (s *Service) storeInto(ctx context.Context, res *batchResult, note *notice, quests []groundedQuest) error {
@@ -277,11 +305,11 @@ func (s *Service) ground(ctx context.Context, quests []skills.SideQuest) ([]grou
 // store stores each grounded Side quest of batchID as a dinner with status
 // 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, a suggestion it
-// repeats passed or a Rotation check dropped, or a Regular it cites renamed
-// or archived. Passed names are read in the same transaction as the store,
-// so a Pass or drop committed before it is never missed.
+// returns the suggestion ids. A Side quest eligible leaves out is not
+// stored: since suggest_side_quests ran, a dinner it repeats was added, a
+// suggestion it repeats passed or a Rotation check dropped, or a Regular
+// it cites renamed or archived. eligible reads in the same transaction as
+// the store, so a Pass or drop committed before it is never missed.
 //
 // When note is not nil and a Side quest is stored, new_suggestions keyed by
 // note, counting and naming those stored, is emitted in the same
@@ -295,28 +323,13 @@ func (s *Service) store(ctx context.Context, batchID string, note *notice, quest
 	var stored []string
 	err := s.st.WithTx(ctx, func(tx *sql.Tx) error {
 		q := db.New(tx)
-		existing, err := q.ListUnarchivedDinners(ctx)
+		kept, err := s.eligible(ctx, q, batchID, quests)
 		if err != nil {
-			return fmt.Errorf("suggest: list dinners for dedupe: %w", err)
+			return err
 		}
-		names, err := q.ListPassedNames(ctx)
-		if err != nil {
-			return fmt.Errorf("suggest: list passed names for dedupe: %w", err)
-		}
-		regulars := map[string][]string{}
-		for _, row := range existing {
-			names = append(names, row.Dinner.Name)
-			if row.Dinner.Status == dinners.StatusRegular {
-				regulars[row.Dinner.Name] = append(regulars[row.Dinner.Name], row.Dinner.ID)
-			}
-		}
-		for _, gq := range quests {
-			cited, ok := citedIDs(gq.quest.CitedRegulars, regulars)
-			if !ok || repeats(gq.quest.Name, names) {
-				s.logger.InfoContext(ctx, "suggested dinner dropped: changed while grounding", "batch_id", batchID)
-				continue
-			}
-			citedJSON, err := json.Marshal(cited)
+		for _, k := range kept {
+			gq := k.groundedQuest
+			citedJSON, err := json.Marshal(k.cited)
 			if err != nil {
 				return fmt.Errorf("suggest: encode cited Regulars: %w", err)
 			}
@@ -354,7 +367,6 @@ func (s *Service) store(ctx context.Context, batchID string, note *notice, quest
 			}); err != nil {
 				return fmt.Errorf("suggest: create suggestion: %w", err)
 			}
-			names = append(names, d.Name)
 			ids = append(ids, id)
 			stored = append(stored, d.Name)
 		}
@@ -372,6 +384,50 @@ func (s *Service) store(ctx context.Context, batchID string, note *notice, quest
 	return ids, nil
 }
 
+// eligibleQuest is a Side quest store would keep, with the ids of the
+// Regulars it cites.
+type eligibleQuest struct {
+	groundedQuest
+	cited []string
+}
+
+// eligible returns, in order, those of batchID's quests that q's dinners
+// and passed names still allow, each with the ids of the Regulars it
+// cites. A Side quest is left out when a dinner it repeats is unarchived,
+// a suggestion it repeats passed or a Rotation check dropped, a Regular it
+// cites is renamed or archived, or an earlier Side quest of quests kept
+// repeats it. store checks with this in its transaction; a notifying
+// batch checks the Side quests it holds with it after each round (see
+// fill).
+func (s *Service) eligible(ctx context.Context, q *db.Queries, batchID string, quests []groundedQuest) ([]eligibleQuest, error) {
+	existing, err := q.ListUnarchivedDinners(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("suggest: list dinners for dedupe: %w", err)
+	}
+	names, err := q.ListPassedNames(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("suggest: list passed names for dedupe: %w", err)
+	}
+	regulars := map[string][]string{}
+	for _, row := range existing {
+		names = append(names, row.Dinner.Name)
+		if row.Dinner.Status == dinners.StatusRegular {
+			regulars[row.Dinner.Name] = append(regulars[row.Dinner.Name], row.Dinner.ID)
+		}
+	}
+	var kept []eligibleQuest
+	for _, gq := range quests {
+		cited, ok := citedIDs(gq.quest.CitedRegulars, regulars)
+		if !ok || repeats(gq.quest.Name, names) {
+			s.logger.InfoContext(ctx, "suggested dinner dropped: changed while grounding", "batch_id", batchID)
+			continue
+		}
+		names = append(names, gq.quest.Name)
+		kept = append(kept, eligibleQuest{groundedQuest: gq, cited: cited})
+	}
+	return kept, nil
+}
+
 // citedIDs returns the ids of the Regulars named cited, false when one
 // names no Regular or more than one.
 func citedIDs(cited []string, regulars map[string][]string) ([]string, bool) {

internal/suggest/suggest_test.go

@@ -660,6 +660,74 @@ func TestBatchKeepsAtMostBatchSize(t *testing.T) {
 	}
 }
 
+// TestBatchReplacesSideQuestMadeIneligibleWhileHeld: a notifying batch
+// holds each round's Side quests until its last round is done; when the
+// household adds a dinner repeating one held from an earlier round while a
+// later round grounds, the batch lets it go and grounds a spare Side quest
+// in its place, so it still stores BatchSize and announces them once.
+func TestBatchReplacesSideQuestMadeIneligibleWhileHeld(t *testing.T) {
+	for _, trigger := range []string{suggest.TriggerScheduled, suggest.TriggerManual} {
+		t.Run(trigger, func(t *testing.T) {
+			e := newEnv(t)
+			e.start(t)
+			// Fajita Dish B is ungroundable, so the first round holds
+			// BatchSize-1 and a second round grounds the next; the last is
+			// the spare a third round grounds in place of Fajita Dish A.
+			var quests, want []string
+			for i := range suggest.BatchSize + 2 {
+				name := fmt.Sprintf("Fajita Dish %c", 'A'+i)
+				e.search.Results[name] = nil
+				if i != 1 {
+					e.recipe(t, name)
+				}
+				quests = append(quests, sideQuest(name, "Like your Chicken Fajitas.", "Chicken Fajitas"))
+				if i > 1 {
+					want = append(want, name)
+				}
+			}
+			e.reply(quests...)
+			// The second round's search waits, so the household adds a
+			// dinner repeating Fajita Dish A, held from the first round,
+			// while that round grounds.
+			second := fmt.Sprintf("Fajita Dish %c", 'A'+suggest.BatchSize)
+			gate := make(chan struct{})
+			e.search.Gates = map[string]chan struct{}{second: gate}
+
+			b, key := e.startBatch(t, trigger)
+			deadline := time.Now().Add(10 * time.Second)
+			for !slices.Contains(e.search.Queries(), second) {
+				if time.Now().After(deadline) {
+					close(gate)
+					t.Fatalf("search for %s never ran", second)
+				}
+				time.Sleep(5 * time.Millisecond)
+			}
+			e.dinner(t, "Fajita Dish A", "regular")
+			close(gate)
+
+			job := e.waitJob(t, b.JobID)
+			if job.Status != jobs.StatusSucceeded {
+				t.Fatalf("batch job %s, error %v", job.Status, job.Error)
+			}
+			var res batchResult
+			if err := json.Unmarshal(job.Result, &res); err != nil {
+				t.Fatal(err)
+			}
+			if len(res.SuggestionIDs) != suggest.BatchSize || res.Dropped != 2 {
+				t.Errorf("batch result = %+v, want %d stored and 2 dropped", res, suggest.BatchSize)
+			}
+			if got := e.deckNames(t); !slices.Equal(got, want) {
+				t.Errorf("deck = %v, want %v", got, want)
+			}
+			rows := notifytest.Rows(t, e.st, notify.NewSuggestions)
+			title := fmt.Sprintf("%d new Side quests are in", suggest.BatchSize)
+			if len(rows) != 1 || rows[0].Key != key || rows[0].Payload.Title != title {
+				t.Errorf("new_suggestions rows = %+v, want one keyed %s titled %q", rows, key, title)
+			}
+		})
+	}
+}
+
 func TestDeckIsOldestFirst(t *testing.T) {
 	e := newEnv(t)
 	e.start(t)

migrations/0018_recipe_item_position_unique.sql

@@ -1,9 +0,0 @@
--- 0018_recipe_item_position_unique: position is the only record of recipe
--- order, so a recipe never has two ingredients or two steps at one
--- position. Writers replace a recipe's items whole, numbered from 0, and
--- the detail read orders by position alone. A database already holding a
--- duplicate fails this migration rather than keep an order nobody can
--- trust.
-
-CREATE UNIQUE INDEX recipe_ingredients_recipe_position ON recipe_ingredients (recipe_id, position);
-CREATE UNIQUE INDEX recipe_steps_recipe_position ON recipe_steps (recipe_id, position);

migrations/queries/recipes.sql

@@ -48,7 +48,7 @@ INSERT INTO recipe_ingredients (
 -- name: ListRecipeIngredients :many
 SELECT * FROM recipe_ingredients
 WHERE recipe_id = ?
-ORDER BY position;
+ORDER BY position, id;
 
 -- name: DeleteRecipeIngredients :exec
 DELETE FROM recipe_ingredients
@@ -64,7 +64,7 @@ INSERT INTO recipe_steps (
 -- name: ListRecipeSteps :many
 SELECT * FROM recipe_steps
 WHERE recipe_id = ?
-ORDER BY position;
+ORDER BY position, id;
 
 -- name: DeleteRecipeSteps :exec
 DELETE FROM recipe_steps