From 515919088a7c67c18a63bfb66999a57ea34ad684 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 22:35:30 -0400 Subject: [PATCH] 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 --- internal/dinners/dinners.go | 19 ++-- internal/dinners/dinners_test.go | 6 +- internal/dinners/export_test.go | 17 +++ internal/dinners/grounding.go | 101 +++++++++++++++--- internal/dinners/grounding_test.go | 161 +++++++++++++++++++++++++++++ internal/server/dinners_test.go | 4 +- web/src/sw.test.ts | 13 ++- web/src/sw/routes.test.ts | 9 +- web/src/sw/routes.ts | 7 +- 9 files changed, 301 insertions(+), 36 deletions(-) diff --git a/internal/dinners/dinners.go b/internal/dinners/dinners.go index 4f956a8..0fabacb 100644 --- a/internal/dinners/dinners.go +++ b/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 - // id last offered by Candidates, so a candidate id is resolved here - // and its URL never leaves the server. - offersMu sync.Mutex - offers map[string]map[string]string + // offersMu guards offers and offerSearches. offers holds, by dinner + // 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. + 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 } diff --git a/internal/dinners/dinners_test.go b/internal/dinners/dinners_test.go index 620fdc6..764f6cb 100644 --- a/internal/dinners/dinners_test.go +++ b/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" - leaks := []string{"c.example", "key", "abcdefghijklmno", "sig", "zyxwvut", "frag"} + const link = "https://c.example/stew-key-abcdefghijklmno?sig=zyxwvut#lookup" + // 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) diff --git a/internal/dinners/export_test.go b/internal/dinners/export_test.go index ca06c60..2767d2b 100644 --- a/internal/dinners/export_test.go +++ b/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. diff --git a/internal/dinners/grounding.go b/internal/dinners/grounding.go index ca668c1..dd24c50 100644 --- a/internal/dinners/grounding.go +++ b/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() - s.offers[row.ID] = urls - s.offersMu.Unlock() + if row.Status != StatusArchived { + keep(urls) + } 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 -// started, or whose page is not a valid recipe link, is a -// *ValidationError. +// 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) { 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() - delete(s.offers, cur.ID) - s.offersMu.Unlock() + s.clearOffers(cur.ID) 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 -// or archived dinner is ErrNotFound or ErrArchived; any other failure -// wraps ErrGroundFailed. +// steps, and records it on the dinner, all in one transaction; the +// dinner's candidates then no longer ground it. A missing or archived +// 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 } diff --git a/internal/dinners/grounding_test.go b/internal/dinners/grounding_test.go index ef2364e..fe2a6e9 100644 --- a/internal/dinners/grounding_test.go +++ b/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. diff --git a/internal/server/dinners_test.go b/internal/server/dinners_test.go index 49e617c..0a8edbd 100644 --- a/internal/server/dinners_test.go +++ b/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) } diff --git a/web/src/sw.test.ts b/web/src/sw.test.ts index 75e6d20..d2f7b01 100644 --- a/web/src/sw.test.ts +++ b/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 () => { - // A household on the v1 worker: its API cache held dinners with the - // pasted recipe_url; its photos and fonts are not in the precache. + it('deletes the v1 and v2 API caches and keeps the current caches', async () => { + // A household that ran the v1 and v2 workers: the v1 API cache held + // 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) }) }) diff --git a/web/src/sw/routes.test.ts b/web/src/sw/routes.test.ts index 62685b3..e4857b8 100644 --- a/web/src/sw/routes.test.ts +++ b/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', () => { - // v1 API responses held dinners with the pasted recipe_url, which can - // carry a personal token; offline, the worker would still serve them. + it('retire only the v1 and v2 API caches', () => { + // v1 API responses held dinners with the pasted recipe_url and v2 ones + // 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', }) diff --git a/web/src/sw/routes.ts b/web/src/sw/routes.ts index e1f0ea3..c741660 100644 --- a/web/src/sw/routes.ts +++ b/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 - // pasted recipe_url. - api: 'v2', + // v2 retired the v1 API cache, whose dinner responses still carried the + // pasted recipe_url; v3 retires the v2 API cache, whose recipe-candidate + // responses used the full URL as candidate_id. + api: 'v3', media: 'v1', fonts: 'v1', } as const