From e07628638e3848c8d708e0c037b19ba623532d72 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 23:53:31 -0400 Subject: [PATCH] Bind candidate offers to the dinner's grounding revision MQ-103-01: offers were bound only to the normalized name and cleared only when it changed, so "Chicken curry" -> "Quick Chicken curry" kept the old candidates live, and a search finishing after renames A -> B -> A committed (before either cleared offers) stored candidates that matched A again. Every actual name edit now raises grounding_revision and clears the dinner's offers. Each offer set records the revision its search read, and a candidate grounds the dinner only while the dinner is still at that revision; GroundDinner already requires the observed revision, so a rename between the check and the write is a conflict. A pending dinner renamed keeps getting a job at its new revision; recipe_url is still cleared only when the normalized name changes. Tests: a same-normalized-name rename refuses the old candidate and attaches no recipe; an in-flight search finishing between the A -> B -> A commits and their offer clearing stores candidates that are refused. Both fail against 3c0a7fd. Maestro task: 4fd737b2-fdcb-48ad-858a-ecdd95595e0c Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 11 ++- internal/dinners/dinners.go | 23 +++--- internal/dinners/export_test.go | 9 +++ internal/dinners/grounding.go | 33 +++++--- internal/dinners/grounding_test.go | 122 +++++++++++++++++++++++++++++ 5 files changed, 172 insertions(+), 26 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index ac09128..8726646 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -96,7 +96,7 @@ Columns ending in `_ct` hold AES-256-GCM ciphertext (via `secretbox`). | `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 when a pending dinner is renamed, so only the latest `ground_dinner` job may write it), updated_by. | +| `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. | | `suggestions` | dinner_id, batch_id, reason, cited_regular_ids (JSON), status, decided_at. | | `week_plans` | week_start (unique), status (`draft` / `synced` / `changed`), ai_note, ai_run_id, calendar_synced_at. | @@ -215,8 +215,9 @@ automatic** (D-021). `no_recipe_found`, `recipe_unusable` (the pasted link), `search_not_configured`, `search_failed`, `ai_failed`, `interrupted` (the server stopped) or `grounding_error`. Each job carries the dinner's `grounding_revision` - from when it was enqueued; renaming a `pending` dinner clears its - `recipe_url`, raises the revision and enqueues a job for the new name. + from when it was enqueued; renaming a dinner raises the revision, and + renaming a `pending` one enqueues a job for the new name, clearing its + `recipe_url` when the normalized name changes. A job for a dinner archived, no longer `pending` or at another revision does no work, and a job only writes a dinner still unarchived, `pending` and at its revision, so a fix-up made meanwhile wins and an earlier job @@ -236,7 +237,9 @@ automatic** (D-021). ranks them; it fetches pages but downloads no photo and runs no model, so `photo_url` is set only for a page already stored. `candidate_id` is an opaque id the server resolves to the page it last offered that dinner - (kept in memory, so a restart means fetching the candidates again); the + (kept in memory, so a restart means fetching the candidates again) at + the `grounding_revision` it was searched at, so a candidate offered + before any rename is refused even if the name is changed back; the page's URL, which may carry a token, is never returned or cached. `POST /api/v1/dinners/{id}/ground` takes exactly one of `{candidate_id}`, `{recipe_url}` (both grounded with `GroundURL`, setting `grounded`) or `{manual: {ingredients[], steps[], minutes, diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 9a9f4b5..6e93d58 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -305,12 +305,14 @@ func (s *Service) List(ctx context.Context, status string) ([]Dinner, error) { // Update applies p to dinner id, recording updatedBy. An invalid patch is a // *ValidationError and a new name repeating another unarchived dinner a -// *DuplicateError; either changes nothing. Renaming a dinner still pending -// clears its recipe_url, raises its grounding_revision and enqueues a fresh -// KindGroundDinner job that grounds the new name: a job queued or running -// for the old name or link carries the old revision, so it can no longer -// write the dinner, and one running is stopped. Renaming any dinner drops -// the candidates offered for its old name: none of them grounds it. +// *DuplicateError; either changes nothing. Renaming a dinner raises its +// grounding_revision and drops the candidates offered for its old name: +// each was offered at an earlier revision, so none of them grounds it. +// Renaming a dinner still pending also enqueues a fresh KindGroundDinner +// job that grounds the new name, and clears its recipe_url when the +// normalized name changes: a job queued or running for the old name or +// link carries the old revision, so it can no longer write the dinner, and +// one running is stopped. func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy string) (Dinner, error) { if updatedBy == "" { return Dinner{}, errors.New("dinners: update: no member to record as updated_by") @@ -355,11 +357,13 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri } params.Name = name params.NormalizedName = harness.Normalize(name) + if name != cur.Name { + params.GroundingRevision++ + } if cur.GroundingStatus == GroundingPending && params.NormalizedName != cur.NormalizedName { // The link was the old dinner's recipe; the new name is // grounded by name, and a link can be given by hand. params.RecipeUrl = sql.NullString{} - params.GroundingRevision++ } } if p.Method != nil { @@ -375,14 +379,15 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri if err != nil { return fmt.Errorf("dinners: update %s: %w", id, err) } - renamed = row.NormalizedName != cur.NormalizedName - regrounding = row.GroundingRevision != cur.GroundingRevision + renamed = row.GroundingRevision != cur.GroundingRevision + regrounding = renamed && cur.GroundingStatus == GroundingPending return nil }) if err != nil { return Dinner{}, err } if renamed { + updateCommitted() s.clearOffers(row.ID) } if regrounding { diff --git a/internal/dinners/export_test.go b/internal/dinners/export_test.go index cf7b488..15deb92 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -22,6 +22,15 @@ func SetOfferLifetime(t *testing.T, d time.Duration) { t.Cleanup(func() { offerLifetime = old }) } +// SetUpdateCommitted runs f in Update once a rename is committed, before +// the dinner's offers are cleared, until t ends. A test calling it must +// not run in parallel. +func SetUpdateCommitted(t *testing.T, f func()) { + old := updateCommitted + updateCommitted = f + t.Cleanup(func() { updateCommitted = old }) +} + // AdvanceClock moves the service's clock d ahead of real time until t // ends, without firing any timer. A test calling it must not run in // parallel. diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index 5fff6f4..df8956c 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -303,13 +303,18 @@ var offerLifetime = 30 * time.Minute // now is the clock offer lifetimes are measured on; tests advance it. var now = time.Now +// updateCommitted runs in Update once a rename is committed, before the +// dinner's offers are cleared; tests use it to act in between. +var updateCommitted = func() {} + // offerSet is the candidates last offered for a dinner: the page URL of -// each candidate id, found by searching the dinner's name, normalized as -// name. They ground the dinner until deadline, and only while it still has -// that name; expiry drops the set from offers once deadline has passed. +// each candidate id, found by searching the dinner's name at its +// grounding_revision revision. They ground the dinner until deadline, and +// only while it is still at that revision, which every rename raises; +// expiry drops the set from offers once deadline has passed. type offerSet struct { urls map[string]string - name string + revision int64 deadline time.Time expiry *time.Timer } @@ -351,16 +356,16 @@ func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error } } if row.Status != StatusArchived { - keep(urls, row.NormalizedName) + keep(urls, row.GroundingRevision) } return out, nil } // trackOffers registers a Candidates search for dinner id. It returns keep, -// which stores urls, found for the normalized name, as the dinner's offers -// for offerLifetime unless clearOffers has run for it since, and done, to -// call when the search ends. -func (s *Service) trackOffers(id string) (keep func(urls map[string]string, name string), done func()) { +// which stores urls, found for the dinner at grounding_revision revision, +// as its offers for offerLifetime unless clearOffers has run for it since, +// and done, to call when the search ends. +func (s *Service) trackOffers(id string) (keep func(urls map[string]string, revision int64), done func()) { search := &offerSearch{} s.offersMu.Lock() if s.offerSearches[id] == nil { @@ -368,14 +373,14 @@ func (s *Service) trackOffers(id string) (keep func(urls map[string]string, name } s.offerSearches[id][search] = struct{}{} s.offersMu.Unlock() - keep = func(urls map[string]string, name string) { + keep = func(urls map[string]string, revision int64) { s.offersMu.Lock() defer s.offersMu.Unlock() if search.stale { return } s.dropOffers(id) - set := &offerSet{urls: urls, name: name, deadline: now().Add(offerLifetime)} + set := &offerSet{urls: urls, revision: revision, deadline: now().Add(offerLifetime)} // expiry waits for offersMu, so it finds set stored even when it // fires at once, and drops set only, never offers stored later. set.expiry = time.AfterFunc(offerLifetime, func() { @@ -431,12 +436,14 @@ func (s *Service) candidateID(id, rawURL string) string { // candidateURL is the page URL of candidate cid last offered for dinner d. // A cid not among the dinner's live offers, or whose page is not a valid // recipe link, is a *ValidationError. Offers past their deadline, or found -// for a name d no longer has, are not live, even before they are dropped. +// at a grounding_revision other than d's, are not live, even before they +// are dropped: d was renamed since they were found, if only to its name +// then. func (s *Service) candidateURL(d db.Dinner, cid string) (string, error) { var raw string var ok bool s.offersMu.Lock() - if set := s.offers[d.ID]; set != nil && set.name == d.NormalizedName && now().Before(set.deadline) { + if set := s.offers[d.ID]; set != nil && set.revision == d.GroundingRevision && now().Before(set.deadline) { raw, ok = set.urls[cid] } s.offersMu.Unlock() diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index 520730b..be0b2a9 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -912,6 +912,128 @@ func TestRenamePendingRefusesOldCandidates(t *testing.T) { } } +// TestRenameSameNormalizedNameRefusesOldCandidates: a rename that leaves +// the normalized name as it was still drops the candidates offered before +// it, and none of them grounds the dinner. +func TestRenameSameNormalizedNameRefusesOldCandidates(t *testing.T) { + t.Parallel() + // Unstarted, so the dinner stays pending with its jobs queued. + e := newUnstartedEnv(t) + e.search.Results["Quick Chicken curry"] = e.search.Results["Chicken curry"] + c, err := e.svc.CreateRegular(t.Context(), dinners.NewRegular{Name: ptr("Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + old := candidateID(t, e, c.ID, "Chicken Curry") + before := e.dinner(t, c.ID) + + d, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Quick Chicken curry")}, member) + if err != nil { + t.Fatal(err) + } + if e.svc.HasOffers(c.ID) { + t.Errorf("service still holds the candidates offered before dinner %s was renamed", c.ID) + } + // A reply is queued, so only refusing the candidate keeps its recipe off. + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + wantStaleCandidate(t, e, c.ID, old) + row := e.dinner(t, c.ID) + if row.RecipeID.Valid { + t.Fatalf("dinner = %+v, want no recipe", row) + } + if row.NormalizedName != before.NormalizedName { + t.Fatalf("normalized name = %q, want %q unchanged", row.NormalizedName, before.NormalizedName) + } + // Still pending, now at the rename's revision, with a job for it. + if d.GroundingStatus != dinners.GroundingPending || row.GroundingRevision != before.GroundingRevision+1 { + t.Fatalf("renamed = %+v, want pending at revision %d", row, before.GroundingRevision+1) + } + var queued int + if err := e.st.Reader.QueryRowContext(t.Context(), + `SELECT count(*) FROM jobs WHERE status = 'queued' AND json_extract(payload, '$.revision') = ?`, + row.GroundingRevision).Scan(&queued); err != nil { + t.Fatal(err) + } + if queued != 1 { + t.Fatalf("queued jobs at revision %d = %d, want 1", row.GroundingRevision, queued) + } +} + +// TestRenameBackDuringCandidatesRefusesThem: a candidate search that read +// a dinner as A and finishes after renames A -> B -> A have committed, but +// before either has cleared the dinner's offers, stores candidates that +// do not ground it, though its name is A again. +func TestRenameBackDuringCandidatesRefusesThem(t *testing.T) { + e := newEnv(t) + id := failedDinner(t, e, "Mystery curry") + e.search.Results["Mystery curry"] = []recipes.Candidate{{URL: curryA}} + asked := len(e.search.Queries()) + gate := make(chan struct{}) + e.search.Gate = gate + + type result struct { + got []dinners.Candidate + err error + } + done := make(chan result, 1) + go func() { + got, err := e.svc.Candidates(t.Context(), id) + done <- result{got, err} + }() + deadline := time.Now().Add(10 * time.Second) + for len(e.search.Queries()) == asked { + if time.Now().After(deadline) { + t.Fatal("candidate search never started") + } + time.Sleep(5 * time.Millisecond) + } + + // The first rename's hook makes the second; the second's lets the + // search finish and tries its candidate, before either clears offers. + var hooks int + var stale error + var groundedBy *dinners.Dinner + dinners.SetUpdateCommitted(t, func() { + hooks++ + switch hooks { + case 1: + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Mystery curry")}, member); err != nil { + t.Error(err) + } + case 2: + close(gate) + r := <-done + if r.err != nil || len(r.got) != 1 { + t.Errorf("candidates = %+v, %v; want curryA", r.got, r.err) + return + } + e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40)) + d, err := e.svc.Ground(t.Context(), id, dinners.Fix{CandidateID: ptr(r.got[0].CandidateID)}, member) + if err == nil { + groundedBy = &d + } + stale = err + } + }) + if _, err := e.svc.Update(t.Context(), id, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil { + t.Fatal(err) + } + if hooks != 2 { + t.Fatalf("renames committed = %d, want 2", hooks) + } + var invalid *dinners.ValidationError + if !errors.As(stale, &invalid) { + t.Fatalf("stale candidate: err = %v, grounded %+v; want *ValidationError", stale, groundedBy) + } + row := e.dinner(t, id) + if row.Name != "Mystery curry" || row.RecipeID.Valid || row.GroundingStatus != dinners.GroundingFailed { + t.Errorf("dinner = %+v, want Mystery curry still failed with no recipe", row) + } + if e.svc.HasOffers(id) { + t.Errorf("service still holds the stale candidates of dinner %s", id) + } +} + // TestCandidatesRunningWhenCleared: a candidate search still running when // its dinner is grounded by hand, renamed or archived stores none of the // candidates it returns, so none of them grounds the dinner.