From bef3f3fd5cb0b6e3e5483ba34c9810fd26fa8904 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 22:50:47 -0400 Subject: [PATCH] Evict expired candidate offers on a timer Each dinner's offered candidates now carry a time.AfterFunc expiry that drops them from the service once offerLifetime passes, so token-bearing candidate URLs are no longer held until some later search sweeps them. Storing new offers or clearing a dinner's offers stops the old timer; the timer drops only the set it was armed for. The expiry runs no goroutine until it fires and touches only the in-memory map. TestCandidatesExpire now waits for the eviction with no further search and checks the candidate no longer grounds; TestExpiredCandidatesNotHeld checks no dinner's expired offers remain held. Co-Authored-By: Claude Opus 5.5 --- internal/dinners/dinners.go | 4 +-- internal/dinners/export_test.go | 7 ++++ internal/dinners/grounding.go | 54 ++++++++++++++++++++---------- internal/dinners/grounding_test.go | 41 ++++++++++++++++++----- 4 files changed, 77 insertions(+), 29 deletions(-) diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 0fabacb..f929396 100644 --- a/internal/dinners/dinners.go +++ b/internal/dinners/dinners.go @@ -151,7 +151,7 @@ type Service struct { // offerSearches holds the Candidates searches running, by dinner id, // so clearing a dinner's offers keeps them from storing theirs. offersMu sync.Mutex - offers map[string]offerSet + offers map[string]*offerSet offerSearches map[string]map[*offerSearch]struct{} } @@ -169,7 +169,7 @@ func NewService(st *store.Store, grounder *recipes.Grounder, runner *jobs.Runner case logger == nil: return nil, errors.New("dinners: nil logger") } - s := &Service{st: st, grounder: grounder, logger: logger, groundings: make(map[string]map[*runningGrounding]struct{}), offerKey: make([]byte, 32), offers: make(map[string]offerSet), offerSearches: make(map[string]map[*offerSearch]struct{})} + s := &Service{st: st, grounder: grounder, logger: logger, groundings: make(map[string]map[*runningGrounding]struct{}), offerKey: make([]byte, 32), offers: make(map[string]*offerSet), offerSearches: make(map[string]map[*offerSearch]struct{})} rand.Read(s.offerKey) // never fails: it crashes the program instead kind, err := jobs.Register(runner, KindGroundDinner, s.runGroundJob, Reasons...) if err != nil { diff --git a/internal/dinners/export_test.go b/internal/dinners/export_test.go index 2767d2b..ef7d545 100644 --- a/internal/dinners/export_test.go +++ b/internal/dinners/export_test.go @@ -31,6 +31,13 @@ func (s *Service) HasOffers(id string) bool { return ok } +// OfferCount is the number of dinners whose candidates the service holds. +func (s *Service) OfferCount() int { + s.offersMu.Lock() + defer s.offersMu.Unlock() + return len(s.offers) +} + // RunGroundJob runs the KindGroundDinner handler for p in the caller's // goroutine, as the job runner would, and returns the grounding status it // reports. diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index dd24c50..0aaed08 100644 --- a/internal/dinners/grounding.go +++ b/internal/dinners/grounding.go @@ -301,10 +301,11 @@ type Candidate struct { var offerLifetime = 30 * time.Minute // offerSet is the candidates last offered for a dinner: the page URL of -// each candidate id, until expires. +// each candidate id. expiry drops the set from offers once offerLifetime +// has passed. type offerSet struct { - urls map[string]string - expires time.Time + urls map[string]string + expiry *time.Timer } // offerSearch is a Candidates search running for a dinner. It is stale @@ -350,9 +351,8 @@ 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 as the dinner's offers unless clearOffers has run for -// it since, and done, to call when the search ends. keep also drops every -// dinner's expired offers, so offers holds only those still live. +// which stores urls 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), done func()) { search := &offerSearch{} s.offersMu.Lock() @@ -362,17 +362,23 @@ func (s *Service) trackOffers(id string) (keep func(urls map[string]string), don s.offerSearches[id][search] = struct{}{} s.offersMu.Unlock() keep = func(urls map[string]string) { - now := time.Now() s.offersMu.Lock() defer s.offersMu.Unlock() - for dinnerID, set := range s.offers { - if !now.Before(set.expires) { - delete(s.offers, dinnerID) + if search.stale { + return + } + s.dropOffers(id) + set := &offerSet{urls: urls} + // 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() { + s.offersMu.Lock() + defer s.offersMu.Unlock() + if s.offers[id] == set { + delete(s.offers, id) } - } - if !search.stale { - s.offers[id] = offerSet{urls: urls, expires: now.Add(offerLifetime)} - } + }) + s.offers[id] = set } done = func() { s.offersMu.Lock() @@ -391,12 +397,21 @@ func (s *Service) trackOffers(id string) (keep func(urls map[string]string), don func (s *Service) clearOffers(id string) { s.offersMu.Lock() defer s.offersMu.Unlock() - delete(s.offers, id) + s.dropOffers(id) for search := range s.offerSearches[id] { search.stale = true } } +// dropOffers drops dinner id's offers and stops their expiry. The caller +// holds offersMu. +func (s *Service) dropOffers(id string) { + if set := s.offers[id]; set != nil { + set.expiry.Stop() + delete(s.offers, id) + } +} + // candidateID is the opaque id of the page at rawURL as a candidate for // dinner id: the same page is offered under the same id until the service // restarts, and the id reveals nothing of the URL. @@ -410,11 +425,14 @@ func (s *Service) candidateID(id, rawURL string) string { // A cid not among the dinner's live offers, or whose page is not a valid // recipe link, is a *ValidationError. func (s *Service) candidateURL(id, cid string) (string, error) { + var raw string + var ok bool s.offersMu.Lock() - set := s.offers[id] - raw, ok := set.urls[cid] + if set := s.offers[id]; set != nil { + raw, ok = set.urls[cid] + } s.offersMu.Unlock() - if ok && time.Now().Before(set.expires) { + if ok { if u, err := checkRecipeURL(raw); err == nil { return u.String(), nil } diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index fe2a6e9..5c21c6d 100644 --- a/internal/dinners/grounding_test.go +++ b/internal/dinners/grounding_test.go @@ -928,24 +928,47 @@ func TestCandidatesRunningWhenCleared(t *testing.T) { } } -// TestCandidatesExpire: candidates ground a dinner only for their -// lifetime; storing another dinner's candidates drops the expired ones. +// TestCandidatesExpire: once their lifetime passes, a dinner's candidates +// are dropped and no longer ground it, with no other search running. func TestCandidatesExpire(t *testing.T) { dinners.SetOfferLifetime(t, 0) e := newEnv(t) id := failedDinner(t, e, "Mystery curry") - other := failedDinner(t, e, "Other curry") e.search.Results["Mystery curry"] = []recipes.Candidate{{URL: curryA}} - e.search.Results["Other curry"] = []recipes.Candidate{{URL: curryB}} cid := candidateID(t, e, id, "Chicken Curry") + waitNoOffers(t, e) wantStaleCandidate(t, e, id, cid) - if !e.svc.HasOffers(id) { - t.Fatalf("candidates of %s not held before another search", id) +} + +// TestExpiredCandidatesNotHeld: the service holds no dinner's candidates +// past their lifetime, however many dinners were offered some. +func TestExpiredCandidatesNotHeld(t *testing.T) { + dinners.SetOfferLifetime(t, 50*time.Millisecond) + e := newEnv(t) + names := map[string]string{"Mystery curry": curryA, "Other curry": curryB} + ids := make([]string, 0, len(names)) + for name, u := range names { + ids = append(ids, failedDinner(t, e, name)) + e.search.Results[name] = []recipes.Candidate{{URL: u}} } - candidateID(t, e, other, "Easy Weeknight Curry") - if e.svc.HasOffers(id) { - t.Errorf("expired candidates of %s still held after another dinner's search", id) + for _, id := range ids { + if _, err := e.svc.Candidates(t.Context(), id); err != nil { + t.Fatal(err) + } + } + waitNoOffers(t, e) +} + +// waitNoOffers waits for the service to hold no dinner's candidates. +func waitNoOffers(t *testing.T, e *env) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for n := e.svc.OfferCount(); n > 0; n = e.svc.OfferCount() { + if time.Now().After(deadline) { + t.Fatalf("service still holds the expired candidates of %d dinners", n) + } + time.Sleep(5 * time.Millisecond) } }