From 0942e6539247a6add7630602d9fedb938fd9d58d Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 11 Oct 2026 08:53:58 -0400 Subject: [PATCH 1/2] Replace a held Side quest made ineligible before a batch's final store A notifying (scheduled or manual) suggestion batch held each round's grounded Side quests and only checked them at its single final store, so a household change during grounding could drop a held card after the fill loop had stopped, leaving the batch short of BatchSize with spare candidates unused (MQ-172-3). The batch now rechecks every held Side quest after each round with the same eligibility check the store uses (eligible, extracted from store), so a later round grounds a spare in its place. The single-transaction store and once-only new_suggestions are unchanged. Narrow ARCHITECTURE.md's "a failed scheduled or manual batch has stored nothing" to failures before the final store, and state that a job that fails recording the batch's end keeps its Side quests and new_suggestions (MQ-172-4). Maestro task 244f9834-faa8-4bb8-b5e6-43677dec0cc0 Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 13 +++- internal/suggest/batch.go | 114 +++++++++++++++++++++++-------- internal/suggest/suggest_test.go | 61 +++++++++++++++++ 3 files changed, 156 insertions(+), 32 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 539863e..51d08d5 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -319,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 diff --git a/internal/suggest/batch.go b/internal/suggest/batch.go index a6e130c..b88d246 100644 --- a/internal/suggest/batch.go +++ b/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) { diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index 6c0052c..c48a8b9 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -660,6 +660,67 @@ func TestBatchKeepsAtMostBatchSize(t *testing.T) { } } +// TestBatchReplacesSideQuestMadeIneligibleWhileHeld: a notifying batch +// holds its first round's Side quests until its last round is done; when +// the household adds a dinner one of them repeats while that 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) + var quests, want []string + for i := range suggest.BatchSize + 1 { + name := fmt.Sprintf("Fajita Dish %c", 'A'+i) + e.recipe(t, name) + quests = append(quests, sideQuest(name, "Like your Chicken Fajitas.", "Chicken Fajitas")) + if i != 0 { + want = append(want, name) + } + } + e.reply(quests...) + // The first round's last search waits, so the household adds a + // dinner repeating Fajita Dish A while that round grounds. + last := fmt.Sprintf("Fajita Dish %c", 'A'+suggest.BatchSize-1) + gate := make(chan struct{}) + e.search.Gates = map[string]chan struct{}{last: gate} + + b, key := e.startBatch(t, trigger) + deadline := time.Now().Add(10 * time.Second) + for !slices.Contains(e.search.Queries(), last) { + if time.Now().After(deadline) { + close(gate) + t.Fatalf("search for %s never ran", last) + } + 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 != 1 { + t.Errorf("batch result = %+v, want %d stored and 1 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) -- 2.55.0 From 8a1c780137537e51841ea5513dddcf1bc1121e2f Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 11 Oct 2026 12:08:48 -0400 Subject: [PATCH 2/2] 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 --- internal/suggest/suggest_test.go | 37 +++++++++++++++++++------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/internal/suggest/suggest_test.go b/internal/suggest/suggest_test.go index 8b84003..24e38ec 100644 --- a/internal/suggest/suggest_test.go +++ b/internal/suggest/suggest_test.go @@ -661,37 +661,44 @@ func TestBatchKeepsAtMostBatchSize(t *testing.T) { } // TestBatchReplacesSideQuestMadeIneligibleWhileHeld: a notifying batch -// holds its first round's Side quests until its last round is done; when -// the household adds a dinner one of them repeats while that 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. +// 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 + 1 { + for i := range suggest.BatchSize + 2 { name := fmt.Sprintf("Fajita Dish %c", 'A'+i) - e.recipe(t, name) + 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 != 0 { + if i > 1 { want = append(want, name) } } e.reply(quests...) - // The first round's last search waits, so the household adds a - // dinner repeating Fajita Dish A while that round grounds. - last := fmt.Sprintf("Fajita Dish %c", 'A'+suggest.BatchSize-1) + // 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{}{last: gate} + 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(), last) { + for !slices.Contains(e.search.Queries(), second) { if time.Now().After(deadline) { close(gate) - t.Fatalf("search for %s never ran", last) + t.Fatalf("search for %s never ran", second) } time.Sleep(5 * time.Millisecond) } @@ -706,8 +713,8 @@ func TestBatchReplacesSideQuestMadeIneligibleWhileHeld(t *testing.T) { if err := json.Unmarshal(job.Result, &res); err != nil { t.Fatal(err) } - if len(res.SuggestionIDs) != suggest.BatchSize || res.Dropped != 1 { - t.Errorf("batch result = %+v, want %d stored and 1 dropped", res, suggest.BatchSize) + 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) -- 2.55.0