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 <noreply@anthropic.com>
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user