diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 6e93d58..0e0c096 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -149,7 +149,7 @@ type Service struct { // id, the candidates last offered by Candidates until they expire, so // a candidate id is resolved here and its URL never leaves the server. // offerSearches holds the Candidates searches running, by dinner id, - // so clearing a dinner's offers keeps them from storing theirs. + // so clearing a dinner's offers keeps them from storing stale ones. offersMu sync.Mutex offers map[string]*offerSet offerSearches map[string]map[*offerSearch]struct{} @@ -388,7 +388,7 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri } if renamed { updateCommitted() - s.clearOffers(row.ID) + s.clearOffers(row.ID, row.GroundingRevision) } if regrounding { s.stopGroundings(row.ID, row.GroundingRevision) @@ -426,7 +426,7 @@ func (s *Service) Archive(ctx context.Context, id, updatedBy string) (Dinner, er return Dinner{}, fmt.Errorf("dinners: archive %s: %w", id, err) } s.stopGroundings(row.ID, math.MaxInt64) - s.clearOffers(row.ID) + s.clearOffers(row.ID, math.MaxInt64) return fromRow(row), nil } diff --git a/internal/dinners/export_test.go b/internal/dinners/export_test.go index 15deb92..b143035 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -49,6 +49,14 @@ func (s *Service) HasOffers(id string) bool { return ok } +// StoreOffers stores urls as dinner id's offers, as a Candidates search +// that read the dinner at grounding_revision revision would on finishing. +func (s *Service) StoreOffers(id string, urls map[string]string, revision int64) { + keep, done := s.trackOffers(id) + defer done() + keep(urls, revision) +} + // OfferCount is the number of dinners whose candidates the service holds. func (s *Service) OfferCount() int { s.offersMu.Lock() diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index df8956c..3166914 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -9,6 +9,7 @@ import ( "encoding/json" "errors" "fmt" + "math" "slices" "strings" "time" @@ -319,10 +320,11 @@ type offerSet struct { expiry *time.Timer } -// offerSearch is a Candidates search running for a dinner. It is stale -// once the dinner's offers are cleared, and then stores none. +// offerSearch is a Candidates search running for a dinner. It stores no +// offers found at a grounding_revision below below, which clearing the +// dinner's offers raises. type offerSearch struct { - stale bool + below int64 } // Candidates searches for dinner id's name and returns up to MaxCandidates @@ -333,7 +335,7 @@ type offerSearch struct { // search that fails wraps recipes.ErrSearchFailed. func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error) { // Tracked before the dinner is read: grounding, renaming or archiving - // it after the read makes this search stale. + // it after the read keeps this search from storing what it found. keep, done := s.trackOffers(id) defer done() row, err := s.get(ctx, id) @@ -363,8 +365,9 @@ func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error // trackOffers registers a Candidates search for dinner id. It returns keep, // 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. +// as its offers for offerLifetime unless clearOffers has run for it since +// with a revision above that, or offers found at a later revision are +// stored, 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() @@ -376,7 +379,12 @@ func (s *Service) trackOffers(id string) (keep func(urls map[string]string, revi keep = func(urls map[string]string, revision int64) { s.offersMu.Lock() defer s.offersMu.Unlock() - if search.stale { + if revision < search.below { + return + } + if set := s.offers[id]; set != nil && set.revision > revision { + // Found at a later revision, so the earlier search's offers + // are already stale. return } s.dropOffers(id) @@ -403,15 +411,20 @@ func (s *Service) trackOffers(id string) (keep func(urls map[string]string, revi return keep, done } -// clearOffers drops dinner id's offers and makes every Candidates search -// running for it stale, so no candidate offered before grounds it. Call -// it once the dinner is grounded by hand, renamed or archived. -func (s *Service) clearOffers(id string) { +// clearOffers drops dinner id's offers found at a grounding_revision below +// below and keeps every Candidates search running for it from storing +// such offers, so none of them grounds it. Offers found at below or above +// are kept. Call it with math.MaxInt64 once the dinner is grounded by hand +// or archived, and with its new revision once it is renamed: a search +// that read the rename meanwhile keeps its offers. +func (s *Service) clearOffers(id string, below int64) { s.offersMu.Lock() defer s.offersMu.Unlock() - s.dropOffers(id) + if set := s.offers[id]; set != nil && set.revision < below { + s.dropOffers(id) + } for search := range s.offerSearches[id] { - search.stale = true + search.below = max(search.below, below) } } @@ -547,7 +560,7 @@ func (s *Service) Ground(ctx context.Context, id string, f Fix, updatedBy string if err != nil { return Dinner{}, fmt.Errorf("%w: dinner %s: record: %w", ErrGroundFailed, id, err) } - s.clearOffers(cur.ID) + s.clearOffers(cur.ID, math.MaxInt64) return fromRow(row), nil } @@ -670,7 +683,7 @@ func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, u if err != nil { return Dinner{}, fmt.Errorf("%w: dinner %s: %w", ErrGroundFailed, id, err) } - s.clearOffers(row.ID) + s.clearOffers(row.ID, math.MaxInt64) return fromRow(row), nil } diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index be0b2a9..fa17cb2 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -959,6 +959,55 @@ func TestRenameSameNormalizedNameRefusesOldCandidates(t *testing.T) { } } +// TestCandidatesBetweenRenameAndClearKept: a candidate search that reads +// a dinner after its rename commits, and stores its candidates before the +// rename clears the dinner's offers, keeps them: they ground the renamed +// dinner, while one offered before the rename does not, even when a search +// that read the dinner before the rename finishes after them. +func TestCandidatesBetweenRenameAndClearKept(t *testing.T) { + // Unstarted, so the dinner stays pending with its jobs queued. + e := newUnstartedEnv(t) + e.search.Results["Beef stew"] = []recipes.Candidate{{URL: stew}} + 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) + + var fresh string + dinners.SetUpdateCommitted(t, func() { + fresh = candidateID(t, e, c.ID, "Beef Stew") + e.svc.StoreOffers(c.ID, map[string]string{old: curryA}, before.GroundingRevision) + }) + if _, err := e.svc.Update(t.Context(), c.ID, dinners.Patch{Name: ptr("Beef stew")}, member); err != nil { + t.Fatal(err) + } + if fresh == "" { + t.Fatal("no candidate search ran between the rename and clearing offers") + } + if !e.svc.HasOffers(c.ID) { + t.Fatalf("clearing offers after the rename dropped the candidates found at its revision for dinner %s", c.ID) + } + wantStaleCandidate(t, e, c.ID, old) + + e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150)) + g, err := e.svc.Ground(t.Context(), c.ID, dinners.Fix{CandidateID: ptr(fresh)}, member) + if err != nil { + t.Fatal(err) + } + if g.GroundingStatus != dinners.GroundingGrounded || g.RecipeID == nil { + t.Fatalf("dinner = %+v, want grounded for the new name", g) + } + rec, err := db.New(e.st.Reader).GetRecipe(t.Context(), *g.RecipeID) + if err != nil { + t.Fatal(err) + } + if rec.SourceUrl != stew { + t.Errorf("recipe source = %s, want %s", rec.SourceUrl, stew) + } +} + // 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