Retire the v2 API cache, bound candidate offers, deflake link leak tests

- Bump the service worker API cache to v3 so activation deletes the v2
  cache, whose recipe-candidate responses used the full page URL as
  candidate_id; test that activation deletes v1 and v2 and keeps v3.
- Give candidate offers a 30-minute lifetime, drop expired offers when
  new ones are stored, and clear a dinner's offers on every hand
  grounding (candidate, link or manual) and on archive. A candidate
  search still running when its dinner is cleared stores nothing, so a
  stale candidate_id gets the same 400 as an unknown one.
- Make the link leak assertions in TestDinnerResponsesHideTokenInLinkPath
  and TestCreateRegularNeverNamesFromLink check phrases a ULID cannot
  contain, so a dinner id holding "KEY" no longer fails them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/dinners/dinners.go

@@ -145,11 +145,14 @@ type Service struct {
 	// offerKey keys the candidate ids Candidates offers, so they are
 	// opaque and stable for this process.
 	offerKey []byte
-	// offersMu guards offers: by dinner id, the page URL of each candidate
+	// offersMu guards offers and offerSearches. offers holds, by dinner
-	// id last offered by Candidates, so a candidate id is resolved here
+	// id, the candidates last offered by Candidates until they expire, so
-	// and its URL never leaves the server.
+	// a candidate id is resolved here and its URL never leaves the server.
-	offersMu sync.Mutex
+	// offerSearches holds the Candidates searches running, by dinner id,
-	offers   map[string]map[string]string
+	// so clearing a dinner's offers keeps them from storing theirs.
+	offersMu      sync.Mutex
+	offers        map[string]offerSet
+	offerSearches map[string]map[*offerSearch]struct{}
 }
 
 // NewService returns a service over st that grounds dinners with grounder
@@ -166,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]map[string]string)}
+	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 {
@@ -395,7 +398,8 @@ func (s *Service) Update(ctx context.Context, id string, p Patch, updatedBy stri
 
 // Archive sets dinner id's status to archived, recording updatedBy. The row
 // is kept, so its name still counts for Side quest dedupe. A
-// KindGroundDinner job running for it is stopped.
+// KindGroundDinner job running for it is stopped, and its candidates no
+// longer ground it.
 func (s *Service) Archive(ctx context.Context, id, updatedBy string) (Dinner, error) {
 	if updatedBy == "" {
 		return Dinner{}, errors.New("dinners: archive: no member to record as updated_by")
@@ -412,6 +416,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)
 	return fromRow(row), nil
 }
 

internal/dinners/dinners_test.go

@@ -16,8 +16,10 @@ import (
 func TestCreateRegularNeverNamesFromLink(t *testing.T) {
 	t.Parallel()
 	e := newUnstartedEnv(t)
-	const link = "https://c.example/stew-key-abcdefghijklmno?sig=zyxwvut#frag"
+	const link = "https://c.example/stew-key-abcdefghijklmno?sig=zyxwvut#lookup"
-	leaks := []string{"c.example", "key", "abcdefghijklmno", "sig", "zyxwvut", "frag"}
+	// Each leak holds a character no ULID has (Crockford base32 has no I, L,
+	// O, U, "-", "." or "/"), so a dinner's id never matches one.
+	leaks := []string{link, "c.example", "stew-key", "abcdefghijklmno", "sig", "zyxwvut", "lookup"}
 	check := func(what, text string) {
 		t.Helper()
 		lower := strings.ToLower(text)

internal/dinners/export_test.go

@@ -14,6 +14,23 @@ func SetGroundTimeout(t *testing.T, d time.Duration) {
 	t.Cleanup(func() { groundTimeout = old })
 }
 
+// SetOfferLifetime sets how long candidates ground a dinner until t ends.
+// A test calling it must not run in parallel.
+func SetOfferLifetime(t *testing.T, d time.Duration) {
+	old := offerLifetime
+	offerLifetime = d
+	t.Cleanup(func() { offerLifetime = old })
+}
+
+// HasOffers reports whether the service holds candidates offered for
+// dinner id.
+func (s *Service) HasOffers(id string) bool {
+	s.offersMu.Lock()
+	defer s.offersMu.Unlock()
+	_, ok := s.offers[id]
+	return ok
+}
+
 // RunGroundJob runs the KindGroundDinner handler for p in the caller's
 // goroutine, as the job runner would, and returns the grounding status it
 // reports.

internal/dinners/grounding.go

@@ -296,11 +296,34 @@ type Candidate struct {
 	Minutes  *int    `json:"minutes"`
 }
 
+// offerLifetime is how long the candidates Candidates offers a dinner
+// ground it; tests shorten it.
+var offerLifetime = 30 * time.Minute
+
+// offerSet is the candidates last offered for a dinner: the page URL of
+// each candidate id, until expires.
+type offerSet struct {
+	urls    map[string]string
+	expires time.Time
+}
+
+// offerSearch is a Candidates search running for a dinner. It is stale
+// once the dinner's offers are cleared, and then stores none.
+type offerSearch struct {
+	stale bool
+}
+
 // Candidates searches for dinner id's name and returns up to MaxCandidates
 // recipe pages to ground it in, best first; the list may be empty. They
 // replace the dinner's earlier candidates: one not offered again no longer
-// grounds it. A search that fails wraps recipes.ErrSearchFailed.
+// grounds it. They ground it for offerLifetime, and not at all once it is
+// grounded by hand or archived, even while this search runs. A search that
+// fails wraps recipes.ErrSearchFailed.
 func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error) {
+	// Tracked before the dinner is read: grounding or archiving it after
+	// the read makes this search stale.
+	keep, done := s.trackOffers(id)
+	defer done()
 	row, err := s.get(ctx, id)
 	if err != nil {
 		return nil, err
@@ -320,12 +343,60 @@ func (s *Service) Candidates(ctx context.Context, id string) ([]Candidate, error
 			out[i].PhotoURL = &u
 		}
 	}
-	s.offersMu.Lock()
+	if row.Status != StatusArchived {
-	s.offers[row.ID] = urls
+		keep(urls)
-	s.offersMu.Unlock()
+	}
 	return out, nil
 }
 
+// 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.
+func (s *Service) trackOffers(id string) (keep func(urls map[string]string), done func()) {
+	search := &offerSearch{}
+	s.offersMu.Lock()
+	if s.offerSearches[id] == nil {
+		s.offerSearches[id] = make(map[*offerSearch]struct{})
+	}
+	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 {
+			s.offers[id] = offerSet{urls: urls, expires: now.Add(offerLifetime)}
+		}
+	}
+	done = func() {
+		s.offersMu.Lock()
+		delete(s.offerSearches[id], search)
+		if len(s.offerSearches[id]) == 0 {
+			delete(s.offerSearches, id)
+		}
+		s.offersMu.Unlock()
+	}
+	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 or archived.
+func (s *Service) clearOffers(id string) {
+	s.offersMu.Lock()
+	defer s.offersMu.Unlock()
+	delete(s.offers, id)
+	for search := range s.offerSearches[id] {
+		search.stale = true
+	}
+}
+
 // 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.
@@ -336,14 +407,14 @@ func (s *Service) candidateID(id, rawURL string) string {
 }
 
 // candidateURL is the page URL of candidate cid last offered for dinner id.
-// A cid not among the dinner's candidates last offered since the service
+// A cid not among the dinner's live offers, or whose page is not a valid
-// started, or whose page is not a valid recipe link, is a
+// recipe link, is a *ValidationError.
-// *ValidationError.
 func (s *Service) candidateURL(id, cid string) (string, error) {
 	s.offersMu.Lock()
-	raw, ok := s.offers[id][cid]
+	set := s.offers[id]
+	raw, ok := set.urls[cid]
 	s.offersMu.Unlock()
-	if ok {
+	if ok && time.Now().Before(set.expires) {
 		if u, err := checkRecipeURL(raw); err == nil {
 			return u.String(), nil
 		}
@@ -443,9 +514,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.offersMu.Lock()
+	s.clearOffers(cur.ID)
-	delete(s.offers, cur.ID)
-	s.offersMu.Unlock()
 	return fromRow(row), nil
 }
 
@@ -467,9 +536,10 @@ func (s *Service) groundRejected(ctx context.Context, id string) error {
 }
 
 // groundManual stores m as dinner id's recipe, with its ingredients and
-// steps, and records it on the dinner, all in one transaction. A missing
+// steps, and records it on the dinner, all in one transaction; the
-// or archived dinner is ErrNotFound or ErrArchived; any other failure
+// dinner's candidates then no longer ground it. A missing or archived
-// wraps ErrGroundFailed.
+// dinner is ErrNotFound or ErrArchived; any other failure wraps
+// ErrGroundFailed.
 func (s *Service) groundManual(ctx context.Context, id string, m ManualRecipe, updatedBy string) (Dinner, error) {
 	var row db.Dinner
 	err := s.st.WithTx(ctx, func(tx *sql.Tx) error {
@@ -567,6 +637,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)
 	return fromRow(row), nil
 }
 

internal/dinners/grounding_test.go

@@ -788,6 +788,167 @@ func TestFixViaCandidate(t *testing.T) {
 	}
 }
 
+// wantStaleCandidate checks that candidate cid no longer grounds dinner
+// id: Ground refuses it as invalid, and the dinner is unchanged.
+func wantStaleCandidate(t *testing.T, e *env, id, cid string) {
+	t.Helper()
+	before := e.dinner(t, id)
+	_, err := e.svc.Ground(t.Context(), id, dinners.Fix{CandidateID: ptr(cid)}, member)
+	var invalid *dinners.ValidationError
+	if !errors.As(err, &invalid) {
+		t.Fatalf("stale candidate: err = %v, want *ValidationError", err)
+	}
+	if after := e.dinner(t, id); after != before {
+		t.Errorf("dinner changed by a stale candidate: %+v, was %+v", after, before)
+	}
+}
+
+// TestGroundingOrArchivingClearsCandidates: once a dinner is grounded by
+// hand — by candidate, link or manual recipe — or archived, the service
+// holds none of its candidates' URLs and none of them grounds it.
+func TestGroundingOrArchivingClearsCandidates(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name string
+		// clear grounds or archives dinner id, offered candidate cid.
+		clear func(t *testing.T, e *env, id, cid string)
+		// archived says the dinner ends archived, which Ground refuses
+		// before it reads a candidate.
+		archived bool
+	}{
+		{"candidate", func(t *testing.T, e *env, id, cid string) {
+			e.model.QueueGenerate(recipestest.NormalizedReply("stovetop", 40))
+			if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{CandidateID: ptr(cid)}, member); err != nil {
+				t.Fatal(err)
+			}
+		}, false},
+		{"link", func(t *testing.T, e *env, id, _ string) {
+			e.model.QueueGenerate(recipestest.NormalizedReply("oven", 150))
+			if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{RecipeURL: ptr(stew)}, member); err != nil {
+				t.Fatal(err)
+			}
+		}, false},
+		{"manual", func(t *testing.T, e *env, id, _ string) {
+			m := &dinners.ManualRecipe{Ingredients: []string{"1 chicken"}, Minutes: ptr(40), Method: ptr("stovetop")}
+			if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{Manual: m}, member); err != nil {
+				t.Fatal(err)
+			}
+		}, false},
+		{"archive", func(t *testing.T, e *env, id, _ string) {
+			if _, err := e.svc.Archive(t.Context(), id, member); err != nil {
+				t.Fatal(err)
+			}
+		}, true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			e := newEnv(t)
+			id := failedDinner(t, e, "Mystery curry")
+			e.search.Results["Mystery curry"] = []recipes.Candidate{{URL: curryA}, {URL: curryB}}
+			cidA := candidateID(t, e, id, "Chicken Curry")
+			cidB := candidateID(t, e, id, "Easy Weeknight Curry")
+
+			tt.clear(t, e, id, cidA)
+			if e.svc.HasOffers(id) {
+				t.Errorf("service still holds the candidates of dinner %s", id)
+			}
+			if !tt.archived {
+				wantStaleCandidate(t, e, id, cidB)
+			}
+		})
+	}
+}
+
+// TestCandidatesRunningWhenCleared: a candidate search still running when
+// its dinner is grounded by hand or archived stores none of the candidates
+// it returns, so none of them grounds the dinner.
+func TestCandidatesRunningWhenCleared(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name  string
+		clear func(t *testing.T, e *env, id string)
+		// archived says the dinner ends archived.
+		archived bool
+	}{
+		{"manual", func(t *testing.T, e *env, id string) {
+			m := &dinners.ManualRecipe{Ingredients: []string{"1 chicken"}, Minutes: ptr(40), Method: ptr("stovetop")}
+			if _, err := e.svc.Ground(t.Context(), id, dinners.Fix{Manual: m}, member); err != nil {
+				t.Fatal(err)
+			}
+		}, false},
+		{"archive", func(t *testing.T, e *env, id string) {
+			if _, err := e.svc.Archive(t.Context(), id, member); err != nil {
+				t.Fatal(err)
+			}
+		}, true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			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)
+			}
+			tt.clear(t, e, id)
+			close(gate)
+			r := <-done
+			if r.err != nil {
+				t.Fatal(r.err)
+			}
+			if len(r.got) != 1 {
+				t.Fatalf("candidates = %+v, want curryA", r.got)
+			}
+			if e.svc.HasOffers(id) {
+				t.Errorf("a search finishing after dinner %s was cleared stored its candidates", id)
+			}
+			if !tt.archived {
+				wantStaleCandidate(t, e, id, r.got[0].CandidateID)
+			}
+		})
+	}
+}
+
+// TestCandidatesExpire: candidates ground a dinner only for their
+// lifetime; storing another dinner's candidates drops the expired ones.
+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")
+
+	wantStaleCandidate(t, e, id, cid)
+	if !e.svc.HasOffers(id) {
+		t.Fatalf("candidates of %s not held before another search", id)
+	}
+	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)
+	}
+}
+
 // TestFixReplacesPendingLink: fixing a Regular added from a link that held
 // no recipe drops that link, so it no longer blocks adding the link again,
 // while the link of the recipe chosen does.

internal/server/dinners_test.go

@@ -292,9 +292,11 @@ func TestDinnerResponsesHideTokenInLinkPath(t *testing.T) {
 	if rec.Code != http.StatusConflict {
 		t.Fatalf("same link again = %d, want 409: %s", rec.Code, rec.Body)
 	}
+	// Each secret holds a character no ULID has (Crockford base32 has no I,
+	// L, O, U, "-", "." or "/"), so the dinner's id never matches one.
 	for _, resp := range []struct{ name, body string }{{"create", created}, {"list", listed}, {"duplicate", rec.Body.String()}} {
 		lower := strings.ToLower(resp.body)
-		for _, secret := range []string{"c.example", "recipes", "key", "abcdefghijklmno"} {
+		for _, secret := range []string{link, "c.example", "recipes/stew", "stew-key", "abcdefghijklmno"} {
 			if strings.Contains(lower, secret) {
 				t.Errorf("%s response contains %q: %s", resp.name, secret, resp.body)
 			}

web/src/sw.test.ts

@@ -15,12 +15,16 @@ describe('service worker activation', () => {
     vi.unstubAllGlobals()
   })
 
-  it('deletes the v1 API cache and keeps the precache, photo and font caches', async () => {
+  it('deletes the v1 and v2 API caches and keeps the current caches', async () => {
-    // A household on the v1 worker: its API cache held dinners with the
+    // A household that ran the v1 and v2 workers: the v1 API cache held
-    // pasted recipe_url; its photos and fonts are not in the precache.
+    // dinners with the pasted recipe_url, the v2 one recipe candidates whose
+    // candidate_id was the full URL. Its photos and fonts are not in the
+    // precache, and the current API cache may already exist.
     const names = [
       'mealquest-precache-v1',
       'mealquest-api-v1',
+      'mealquest-api-v2',
+      'mealquest-api-v3',
       'mealquest-media-v1',
       'mealquest-fonts-v1',
       'other-app-cache',
@@ -42,6 +46,7 @@ describe('service worker activation', () => {
 
     expect(pending).toHaveLength(1)
     expect(cacheStorage.delete).toHaveBeenCalledWith('mealquest-api-v1')
-    expect(cacheStorage.delete).toHaveBeenCalledTimes(1)
+    expect(cacheStorage.delete).toHaveBeenCalledWith('mealquest-api-v2')
+    expect(cacheStorage.delete).toHaveBeenCalledTimes(2)
   })
 })

web/src/sw/routes.test.ts

@@ -147,12 +147,13 @@ describe('cache names', () => {
     for (const name of Object.values(cacheNames)) expect(name).toMatch(/^mealquest-/)
   })
 
-  it('retire only the v1 API cache', () => {
+  it('retire only the v1 and v2 API caches', () => {
-    // v1 API responses held dinners with the pasted recipe_url, which can
+    // v1 API responses held dinners with the pasted recipe_url and v2 ones
-    // carry a personal token; offline, the worker would still serve them.
+    // recipe candidates whose candidate_id was the full URL; either can carry
+    // a personal token, and offline the worker would still serve them.
     // The precache, photos and fonts keep their v1 caches.
     expect(cacheNames).toEqual({
-      api: 'mealquest-api-v2',
+      api: 'mealquest-api-v3',
       media: 'mealquest-media-v1',
       fonts: 'mealquest-fonts-v1',
     })

web/src/sw/routes.ts

@@ -29,9 +29,10 @@ export interface RouteRequest {
 // photos and fonts stay available offline.
 export const cacheVersions = {
   precache: 'v1',
-  // v2 retires the v1 API cache, whose dinner responses still carried the
+  // v2 retired the v1 API cache, whose dinner responses still carried the
-  // pasted recipe_url.
+  // pasted recipe_url; v3 retires the v2 API cache, whose recipe-candidate
-  api: 'v2',
+  // responses used the full URL as candidate_id.
+  api: 'v3',
   media: 'v1',
   fonts: 'v1',
 } as const