From 03f799c555ce58a7d86957ced289c186a20c06bb Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Sat, 1 Aug 2026 15:04:44 -0700 Subject: [PATCH 1/2] Cut Google Maps spend: cache reverse-geocode, trim Details mask, skip redundant confirm Details, persist closures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four API-cost fixes plus one freshness fix that costs nothing, from the 2026-08-01 geo cost/freshness audit. No wire-contract changes. - Reverse-geocode results are now cached in Redis per ~8 km search cell (30-day expiry, matching Geocoding caching terms). This was the ONLY Google call a warm nearby scan made — processLocation buys it on every request — so a warm scan's Google spend drops to zero. The RedisClient.ReverseGeocode stub is now real; processLocation and the reverse-geocoding endpoint route through the cached path. - detailed_search_fields drops name and user_ratings_total: no Details consumer reads either (both arrive free with every Nearby/Text Search result), and user_ratings_total alone pulls every Details call into the Atmosphere billing tier. The AddUserRatingsTotal migration passes its own field list and is unaffected. Pinned by TestDetailedSearchFieldsMask. - place-search confirm skips its Place Details call when the cached record's details are current (same placeDetailsAreCurrent rule the nearby path trusts) — re-confirming a place we already hold was a full-price max-tier call for data restoreCachedDetails restores anyway. - Stale-photo recovery requests only the photos field instead of the full Details mask. - Cold searches now PERSIST permanently-closed places instead of filtering them out before the cache write. The old order discarded the closure signal entirely: the stale record kept OPERATIONAL status and cache membership forever. The response-side filter is unchanged (closures never reach callers); the read-side Operational filter now retires them from cache serves at zero extra API cost. Not included (deliberately): the hoursKnown wire flag (API change), text-search caching and timeout salvage (larger changes), and the MapsLastSearchTime dead-field purge (Redis ops, not code). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- config/config.yml | 6 +- config_yaml_test.go | 46 ++++++++ .../add_searched_place_currency_test.go | 110 ++++++++++++++++++ iowrappers/closure_persistence_test.go | 93 +++++++++++++++ iowrappers/photos_client.go | 4 +- iowrappers/poi_searcher.go | 72 ++++++++---- iowrappers/redis_client.go | 37 +++++- iowrappers/reverse_geocode_cache_test.go | 106 +++++++++++++++++ iowrappers/text_search.go | 22 +++- planner/planner.go | 2 +- 10 files changed, 461 insertions(+), 37 deletions(-) create mode 100644 config_yaml_test.go create mode 100644 iowrappers/add_searched_place_currency_test.go create mode 100644 iowrappers/closure_persistence_test.go create mode 100644 iowrappers/reverse_geocode_cache_test.go diff --git a/config/config.yml b/config/config.yml index 812dbbb5d..7ccc53385 100644 --- a/config/config.yml +++ b/config/config.yml @@ -1,12 +1,14 @@ server: google_maps: + # Every field is billed per Details call, tiered by field group — see + # TestDetailedSearchFieldsMask for why name and user_ratings_total are + # deliberately absent (nothing reads them from a Details response; both + # arrive free with every Nearby/Text Search result). detailed_search_fields: - - name - opening_hours - formatted_address - adr_address - url - - user_ratings_total - editorial_summary - photos plan_solver: diff --git a/config_yaml_test.go b/config_yaml_test.go new file mode 100644 index 000000000..4440c22a5 --- /dev/null +++ b/config_yaml_test.go @@ -0,0 +1,46 @@ +package main + +import ( + "os" + "testing" + + "gopkg.in/yaml.v3" +) + +// Pins the Place Details field mask in config/config.yml. Every field here is billed per Details +// call, tiered by field group (Basic / Contact / Atmosphere on the legacy API), so an unused +// field is pure spend: +// - name and user_ratings_total must stay OUT: neither is read from a Details response on any +// path (both already arrive free with every Nearby/Text Search result), and +// user_ratings_total alone pulls the call into the Atmosphere tier. The AddUserRatingsTotal +// admin migration passes its own single-field list and is unaffected. +// - the remaining fields are load-bearing: opening_hours (open-now filtering), +// formatted_address/adr_address (display + address parsing), url (also the Details-freshness +// signal — see iowrappers/data_migrations.go detailsSourcedFields), editorial_summary (trip +// planner), photos (place photos + confirm gap-fill). +func TestDetailedSearchFieldsMask(t *testing.T) { + raw, err := os.ReadFile("config/config.yml") + if err != nil { + t.Fatalf("reading config/config.yml: %v", err) + } + var configs Configurations + if err := yaml.Unmarshal(raw, &configs); err != nil { + t.Fatalf("unmarshal config.yml: %v", err) + } + + fields := make(map[string]bool) + for _, f := range configs.Server.GoogleMaps.DetailedSearchFields { + fields[f] = true + } + + for _, banned := range []string{"name", "user_ratings_total"} { + if fields[banned] { + t.Errorf("detailed_search_fields contains %q, which no Details consumer reads — it only adds billing tier", banned) + } + } + for _, required := range []string{"opening_hours", "formatted_address", "adr_address", "url", "editorial_summary", "photos"} { + if !fields[required] { + t.Errorf("detailed_search_fields is missing load-bearing field %q", required) + } + } +} diff --git a/iowrappers/add_searched_place_currency_test.go b/iowrappers/add_searched_place_currency_test.go new file mode 100644 index 000000000..0c65991cb --- /dev/null +++ b/iowrappers/add_searched_place_currency_test.go @@ -0,0 +1,110 @@ +package iowrappers + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/weihesdlegend/Vacation-planner/POI" + "googlemaps.github.io/maps" +) + +// These tests pin when a confirm buys a Place Details call. A Details call is the single most +// expensive Google request the service makes (max field tier), and a re-confirm of a place whose +// stored record already carries current Details-sourced fields holds all the data the confirm +// needs — the same placeDetailsAreCurrent rule the nearby-search path already trusts. + +func countingEnricher(calls *int) placeDetailsEnricher { + return func(ctx context.Context, placeID string) (maps.PlaceDetailsResult, error) { + *calls++ + return maps.PlaceDetailsResult{}, errors.New("no network in this test") + } +} + +func TestConfirmSkipsDetailsWhenCachedRecordIsCurrent(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-current" + stored := POI.Place{ + ID: placeID, + Name: "City History Museum", + LocationType: POI.LocationType("museum"), + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + URL: "https://maps.google.com/?cid=42", // proof a Details call landed (detailsSourcedFields) + LastUpdatedAt: time.Now().Format(time.RFC3339), + } + s.redisClient.SetPlacesAddGeoLocations(ctx, []POI.Place{stored}) + + stashCandidate(t, s, ctx, POI.Place{ + ID: placeID, + Name: "City History Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + }) + + calls := 0 + result, err := s.addSearchedPlaceToCache(ctx, placeID, countingEnricher(&calls)) + if err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if calls != 0 { + t.Errorf("enricher called %d times for a current cached record, want 0 — this is a billed Place Details call", calls) + } + if !result.AlreadyCached { + t.Error("AlreadyCached = false, want true") + } + if result.Place.URL != stored.URL { + t.Errorf("Place.URL = %q, want the cached record's %q restored", result.Place.URL, stored.URL) + } +} + +func TestConfirmBuysDetailsWhenCachedRecordIsStale(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-stale" + stale := time.Now().Add(-(PlaceDetailsRefreshDuration + 24*time.Hour)) + s.redisClient.SetPlacesAddGeoLocations(ctx, []POI.Place{{ + ID: placeID, + Name: "Old Museum", + LocationType: POI.LocationType("museum"), + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + URL: "https://maps.google.com/?cid=43", + LastUpdatedAt: stale.Format(time.RFC3339), + }}) + + stashCandidate(t, s, ctx, POI.Place{ + ID: placeID, + Name: "Old Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + }) + + calls := 0 + if _, err := s.addSearchedPlaceToCache(ctx, placeID, countingEnricher(&calls)); err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if calls != 1 { + t.Errorf("enricher called %d times for a stale cached record, want 1", calls) + } +} + +func TestConfirmBuysDetailsWhenNotCached(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + placeID := "museum-uncached" + stashCandidate(t, s, ctx, POI.Place{ + ID: placeID, + Name: "Brand New Museum", + Types: []string{"museum", "point_of_interest", "establishment"}, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + }) + + calls := 0 + if _, err := s.addSearchedPlaceToCache(ctx, placeID, countingEnricher(&calls)); err != nil { + t.Fatalf("addSearchedPlaceToCache: %v", err) + } + if calls != 1 { + t.Errorf("enricher called %d times for an uncached place, want 1", calls) + } +} diff --git a/iowrappers/closure_persistence_test.go b/iowrappers/closure_persistence_test.go new file mode 100644 index 000000000..82d03b35a --- /dev/null +++ b/iowrappers/closure_persistence_test.go @@ -0,0 +1,93 @@ +package iowrappers + +import ( + "testing" + + "github.com/weihesdlegend/Vacation-planner/POI" +) + +// Pins that a cold search PERSISTS permanently-closed places while still excluding them from the +// response. The previous behavior filtered non-Operational results BEFORE the cache write, which +// discarded the closure signal entirely: the stale record kept Status OPERATIONAL forever and the +// place kept being served from cache. Persisting the closure lets the read-side Operational +// filters (RedisClient.NearbySearch) retire the place everywhere, at zero extra API cost. +func TestColdSearchPersistsClosuresButExcludesThemFromResponse(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + open := POI.Place{ + ID: "open-1", + Name: "Open Diner", + LocationType: POI.LocationType("restaurant"), + Status: POI.Operational, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + closed := POI.Place{ + ID: "closed-1", + Name: "Shuttered Grill", + LocationType: POI.LocationType("restaurant"), + Status: POI.ClosedPermanently, + Location: POI.Location{Latitude: 37.41, Longitude: -122.11}, + } + req := &PlaceSearchRequest{ + PlaceCat: POI.PlaceCategoryEatery, + BusinessStatus: POI.Operational, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + + got := s.persistAndFilterSearchResults(ctx, req, []POI.Place{open, closed}) + + if len(got) != 1 || got[0].ID != "open-1" { + t.Fatalf("response = %+v, want only open-1 (closed places excluded from the response)", got) + } + + cached, err := s.redisClient.CachedPlaces(ctx, []string{"open-1", "closed-1"}) + if err != nil { + t.Fatalf("CachedPlaces: %v", err) + } + if _, ok := cached["open-1"]; !ok { + t.Error("open-1 was not persisted") + } + stored, ok := cached["closed-1"] + if !ok { + t.Fatal("closed-1 was not persisted — the closure signal was discarded, the pre-write-filter bug") + } + if stored.Status != POI.ClosedPermanently { + t.Errorf("closed-1 Status = %q, want %q recorded", stored.Status, POI.ClosedPermanently) + } + + // The read path must retire it: an Operational-filtered cache read excludes the closed place. + readReq := &PlaceSearchRequest{ + PlaceCat: POI.PlaceCategoryEatery, + BusinessStatus: POI.Operational, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + Radius: 1000, + MinNumResults: 1, + } + fromCache, err := s.redisClient.NearbySearch(ctx, readReq) + if err != nil { + t.Fatalf("RedisClient.NearbySearch: %v", err) + } + for _, p := range fromCache { + if p.ID == "closed-1" { + t.Error("closed-1 served from an Operational-filtered cache read") + } + } +} + +func TestPersistAndFilterWithoutStatusFilterReturnsEverything(t *testing.T) { + s, ctx := newAddSearchedPlaceFixture(t) + + places := []POI.Place{ + {ID: "a", Name: "A", LocationType: POI.LocationType("restaurant"), Status: POI.Operational, Location: POI.Location{Latitude: 37.4, Longitude: -122.1}}, + {ID: "b", Name: "B", LocationType: POI.LocationType("restaurant"), Status: POI.ClosedPermanently, Location: POI.Location{Latitude: 37.41, Longitude: -122.11}}, + } + req := &PlaceSearchRequest{ + PlaceCat: POI.PlaceCategoryEatery, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + } + + got := s.persistAndFilterSearchResults(ctx, req, places) + if len(got) != 2 { + t.Fatalf("response has %d places, want 2 — no BusinessStatus filter was requested", len(got)) + } +} diff --git a/iowrappers/photos_client.go b/iowrappers/photos_client.go index 6ee61af10..c2709a678 100644 --- a/iowrappers/photos_client.go +++ b/iowrappers/photos_client.go @@ -139,7 +139,9 @@ func (c *MapsPhotoClient) GetPhotoURL(ctx context.Context, photoRef string, plac // Acquire semaphore for API rate limiting c.mapsClient.apiSemaphore <- struct{}{} - r, err = c.mapsClient.PlaceDetailedSearch(ctx, placeId, c.mapsClient.DetailedSearchFields) + // Only the photo reference is needed here — the full DetailedSearchFields mask + // would bill the Contact/Atmosphere field tiers for data this path throws away. + r, err = c.mapsClient.PlaceDetailedSearch(ctx, placeId, []string{"photos"}) <-c.mapsClient.apiSemaphore // Release semaphore if err != nil { return "", err diff --git a/iowrappers/poi_searcher.go b/iowrappers/poi_searcher.go index f07c53651..fcc72dc28 100644 --- a/iowrappers/poi_searcher.go +++ b/iowrappers/poi_searcher.go @@ -148,9 +148,20 @@ func (s *PoiSearcher) Geocode(context context.Context, query *GeocodeQuery) (lat return } +// ReverseGeocode resolves city-level info for a coordinate, consulting the per-cell Redis cache +// before Google. This runs on EVERY nearby scan (processLocation), and on a warm place cache it +// was the only Google call left — caching it takes a warm scan's Google spend to zero. func (s *PoiSearcher) ReverseGeocode(ctx context.Context, lat, lng float64) (*GeocodeQuery, error) { Logger.Debugf("PoiSearcher ->ReverseGeocode: decoding latitude %.2f, longitude %.2f", lat, lng) - return s.mapsClient.ReverseGeocode(ctx, lat, lng) + if cached, err := s.redisClient.ReverseGeocode(ctx, lat, lng); err == nil { + return cached, nil + } + query, err := s.mapsClient.ReverseGeocode(ctx, lat, lng) + if err != nil { + return nil, err + } + s.redisClient.SetReverseGeocode(ctx, lat, lng, *query) + return query, nil } // canServeFromCache decides whether cached places can satisfy a request without an external @@ -248,27 +259,50 @@ func (s *PoiSearcher) NearbySearch(context context.Context, request *PlaceSearch utils.LogErrorWithLevel(s.redisClient.SetMapsLastSearchTime(context, lat, lng, request.PlaceCat, request.PriceLevel, currentTime.Format(time.RFC3339)), utils.LogError) } - if request.Keyword != "" && request.StrictNameMatch { + places = append(places, s.persistAndFilterSearchResults(context, request, newPlaces)...) + + return places, nil +} + +// persistAndFilterSearchResults writes a cold search's results to the cache and returns the slice +// the caller may serve. The write deliberately includes non-Operational places: a permanent +// closure reported by Google is a signal we must PERSIST (so the read-side Operational filters +// retire the stale record everywhere), not discard — the previous pre-write filter meant a closed +// place kept its OPERATIONAL record and cache membership forever. The response filter then keeps +// closures out of what the caller sees, same as before. +func (s *PoiSearcher) persistAndFilterSearchResults(ctx context.Context, req *PlaceSearchRequest, newPlaces []POI.Place) []POI.Place { + if req.Keyword != "" && req.StrictNameMatch { // drop keyword-search results that are merely related to the brand (Google Maps // matches keywords against reviews and other content, not just names) before they // reach the brand-scoped cache - newPlaces = Filter(newPlaces, func(place POI.Place) bool { return MatchesBrandName(place.Name, request.Keyword) }) + newPlaces = Filter(newPlaces, func(place POI.Place) bool { return MatchesBrandName(place.Name, req.Keyword) }) } // safeguard on accessing elements in a nil slice if len(newPlaces) > 0 { - // update Redis with all the new places obtained - if request.Keyword != "" { - s.redisClient.SetPlacesAddGeoLocationsForBrand(context, request.Keyword, newPlaces) + // update Redis with all the new places obtained, closures included + if req.Keyword != "" { + s.redisClient.SetPlacesAddGeoLocationsForBrand(ctx, req.Keyword, newPlaces) } else { - s.UpdateRedis(context, newPlaces) + s.UpdateRedis(ctx, newPlaces) } + } - // include places from cache in the result - places = append(places, newPlaces...) + if req.BusinessStatus == POI.Operational { + totalPlacesCount := len(newPlaces) + newPlaces = Filter(newPlaces, func(place POI.Place) bool { return place.Status == POI.Operational }) + Logger.Debugf("%d places out of %d left after business status filtering", len(newPlaces), totalPlacesCount) } - return places, nil + if uint(len(newPlaces)) < req.MinNumResults { + Logger.Debugf("Found %d POI results for place type %s, less than requested number of %d", + len(newPlaces), req.PlaceCat, req.MinNumResults) + } + if len(newPlaces) == 0 { + Logger.Debugf("No qualified POI result found in the given location %v, radius %d, and place type: %s. The location may be invalid", + req.Location, req.Radius, req.PlaceCat) + } + return newPlaces } // processLocation performs reverse geocoding for precise location to find city-level information and performs geocoding to find precise latitude and longitude values @@ -281,7 +315,7 @@ func (s *PoiSearcher) processLocation(ctx context.Context, req *PlaceSearchReque } if req.UsePreciseLocation { Logger.Debugf("->NearbySearch: using precise location") - geoQuery, err := s.GetMapsClient().ReverseGeocode(ctx, req.Location.Latitude, req.Location.Longitude) + geoQuery, err := s.ReverseGeocode(ctx, req.Location.Latitude, req.Location.Longitude) if err != nil { return err } @@ -319,20 +353,8 @@ func (s *PoiSearcher) searchPlacesWithMaps(ctx context.Context, req *PlaceSearch return nil, err } - if req.BusinessStatus == POI.Operational { - totalPlacesCount := len(places) - places = Filter(places, func(place POI.Place) bool { return place.Status == POI.Operational }) - Logger.Debugf("%d places out of %d left after business status filtering", len(places), totalPlacesCount) - } - - if uint(len(places)) < req.MinNumResults { - Logger.Debugf("Found %d POI results for place type %s, less than requested number of %d", - len(places), req.PlaceCat, req.MinNumResults) - } - if len(places) == 0 { - Logger.Debugf("No qualified POI result found in the given location %v, radius %d, and place type: %s. The location may be invalid", - req.Location, req.Radius, req.PlaceCat) - } + // No status filtering here: closures must reach the cache write so they are persisted — + // persistAndFilterSearchResults owns both the write and the response-side filter. return places, nil } diff --git a/iowrappers/redis_client.go b/iowrappers/redis_client.go index 3cb74c305..ab7cb2969 100644 --- a/iowrappers/redis_client.go +++ b/iowrappers/redis_client.go @@ -673,8 +673,41 @@ func (r *RedisClient) Geocode(context context.Context, query *GeocodeQuery) (lat return } -func (r *RedisClient) ReverseGeocode(context.Context, float64, float64) (*GeocodeQuery, error) { - return nil, errors.New("->ReverseGeocode: not implemented for the RedisClient") +// ReverseGeocodeExpiration bounds how long a cached reverse-geocode result is served. City-level +// info is effectively static, but Google's Geocoding terms only allow temporary caching — 30 days +// keeps the cache self-cleaning and policy-friendly. +const ReverseGeocodeExpiration = 30 * 24 * time.Hour + +// reverseGeocodeRedisKey keys reverse-geocode results by the same ~8 km search cell the +// MapsLastSearchTime markers use: the result only carries country/admin1/locality, which is +// stable at that granularity. +func reverseGeocodeRedisKey(lat, lng float64) string { + return "geocode:reverse:" + POI.EncodeSearchCell(lat, lng) +} + +func (r *RedisClient) ReverseGeocode(ctx context.Context, lat, lng float64) (*GeocodeQuery, error) { + res, err := r.client.Get(ctx, reverseGeocodeRedisKey(lat, lng)).Result() + if err != nil { + return nil, err + } + var query GeocodeQuery + if err = json.Unmarshal([]byte(res), &query); err != nil { + return nil, err + } + return &query, nil +} + +// SetReverseGeocode caches a reverse-geocode result for the cell containing (lat, lng). +// Best-effort like SetGeocode: a write failure only means the next request buys the geocode again. +func (r *RedisClient) SetReverseGeocode(ctx context.Context, lat, lng float64, query GeocodeQuery) { + payload, err := json.Marshal(query) + if err != nil { + utils.LogErrorWithLevel(err, utils.LogError) + return + } + utils.LogErrorWithLevel( + r.client.Set(ctx, reverseGeocodeRedisKey(lat, lng), payload, ReverseGeocodeExpiration).Err(), + utils.LogError) } func (r *RedisClient) SetGeocode(context context.Context, query GeocodeQuery, lat float64, lng float64, originalQuery GeocodeQuery) { diff --git a/iowrappers/reverse_geocode_cache_test.go b/iowrappers/reverse_geocode_cache_test.go new file mode 100644 index 000000000..64f4560b2 --- /dev/null +++ b/iowrappers/reverse_geocode_cache_test.go @@ -0,0 +1,106 @@ +package iowrappers + +import ( + "context" + "net/url" + "testing" + "time" + + "github.com/alicebob/miniredis/v2" + "github.com/weihesdlegend/Vacation-planner/POI" +) + +// newReverseGeocodeFixture builds a RedisClient backed by its own miniredis instance, mirroring +// add_searched_place_test.go's own-server-per-test harness. +func newReverseGeocodeFixture(t *testing.T) (*RedisClient, *miniredis.Miniredis, context.Context) { + t.Helper() + svr, err := miniredis.Run() + if err != nil { + t.Fatalf("miniredis.Run: %v", err) + } + t.Cleanup(svr.Close) + + redisURL, _ := url.Parse("redis://" + svr.Addr()) + if err := CreateLogger(); err != nil { + t.Fatalf("CreateLogger: %v", err) + } + return CreateRedisClient(redisURL), svr, context.Background() +} + +func TestReverseGeocodeCacheRoundTrip(t *testing.T) { + r, _, ctx := newReverseGeocodeFixture(t) + want := GeocodeQuery{City: "Mountain View", AdminAreaLevelOne: "CA", Country: "US"} + + r.SetReverseGeocode(ctx, 37.4001, -122.0801, want) + + // Nearby coordinates land in the same ~8 km search cell and must hit. + got, err := r.ReverseGeocode(ctx, 37.4002, -122.0802) + if err != nil { + t.Fatalf("ReverseGeocode after Set: %v", err) + } + if *got != want { + t.Fatalf("ReverseGeocode = %+v, want %+v", *got, want) + } +} + +func TestReverseGeocodeCacheMiss(t *testing.T) { + r, _, ctx := newReverseGeocodeFixture(t) + + if _, err := r.ReverseGeocode(ctx, 37.4, -122.08); err == nil { + t.Fatal("ReverseGeocode on an empty cache must return an error") + } + + // A hit in one cell must not leak into another cell. + r.SetReverseGeocode(ctx, 37.4, -122.08, GeocodeQuery{City: "Mountain View", Country: "US"}) + if _, err := r.ReverseGeocode(ctx, 40.7, -74.0); err == nil { + t.Fatal("ReverseGeocode for a different cell must miss") + } +} + +func TestReverseGeocodeCacheExpires(t *testing.T) { + r, svr, ctx := newReverseGeocodeFixture(t) + r.SetReverseGeocode(ctx, 37.4, -122.08, GeocodeQuery{City: "Mountain View", Country: "US"}) + + svr.FastForward(ReverseGeocodeExpiration + time.Hour) + + if _, err := r.ReverseGeocode(ctx, 37.4, -122.08); err == nil { + t.Fatal("ReverseGeocode must miss after the cache entry expires") + } +} + +// A cached cell must satisfy PoiSearcher.ReverseGeocode without any Google call: the fixture's +// mapsClient is nil, so reaching for Google would panic — returning the cached value is the proof +// the cache is consulted first. This is the call every warm nearby scan makes. +func TestPoiSearcherReverseGeocodeServesFromCache(t *testing.T) { + r, _, ctx := newReverseGeocodeFixture(t) + s := &PoiSearcher{redisClient: r} + want := GeocodeQuery{City: "Los Altos", AdminAreaLevelOne: "CA", Country: "US"} + r.SetReverseGeocode(ctx, 37.379, -122.117, want) + + got, err := s.ReverseGeocode(ctx, 37.379, -122.117) + if err != nil { + t.Fatalf("ReverseGeocode: %v", err) + } + if *got != want { + t.Fatalf("ReverseGeocode = %+v, want %+v", *got, want) + } +} + +// processLocation's precise-location branch is the nearby-scan entry point for the reverse +// geocode — it must go through the searcher's cached path, not straight to the maps client. +func TestProcessLocationPreciseUsesCachedReverseGeocode(t *testing.T) { + r, _, ctx := newReverseGeocodeFixture(t) + s := &PoiSearcher{redisClient: r} + r.SetReverseGeocode(ctx, 37.379, -122.117, GeocodeQuery{City: "Los Altos", AdminAreaLevelOne: "CA", Country: "US"}) + + req := &PlaceSearchRequest{ + Location: POI.Location{Latitude: 37.379, Longitude: -122.117}, + UsePreciseLocation: true, + } + if err := s.processLocation(ctx, req); err != nil { + t.Fatalf("processLocation: %v", err) + } + if req.Location.City != "Los Altos" || req.Location.Country != "US" { + t.Fatalf("processLocation resolved %+v, want cached Los Altos/US", req.Location) + } +} diff --git a/iowrappers/text_search.go b/iowrappers/text_search.go index 92310a66a..66f733522 100644 --- a/iowrappers/text_search.go +++ b/iowrappers/text_search.go @@ -246,8 +246,10 @@ func newPlaceDetailsEnricher(sem chan struct{}, fields []string, search placeDet // no geo bucket member, no place_details record. // 3. Read whether the place was already cached, BEFORE this confirm writes anything, so // AlreadyCached reflects prior state. -// 4. Best-effort enrich via Place Details for hours/address/URL/summary; a failure here is -// logged and the lean (stash-only) record is used instead of failing the confirm. +// 4. Best-effort enrich via Place Details for hours/address/URL/summary — SKIPPED entirely when +// the already-cached record's details are current (placeDetailsAreCurrent), since step 5 +// restores those fields anyway and the call is the most expensive Google request we make. A +// failure is logged and the lean (stash-only) record is used instead of failing the confirm. // 5. Restore any Details-sourced fields (esp. real opening hours) a previously cached record had // that this pass didn't obtain, so confirming an already-cached place can never regress it to // placeholder data. @@ -276,11 +278,19 @@ func (s *PoiSearcher) addSearchedPlaceToCache(ctx context.Context, placeID strin alreadyCached := len(cached) > 0 place := candidate - details, enrichErr := enrich(ctx, placeID) - if enrichErr != nil { - Logger.Errorf("addSearchedPlaceToCache: Place Details enrich failed for %s, continuing with the lean stashed record: %v", placeID, enrichErr) + // Skip the (max-tier) Place Details call when the stored record already carries current + // Details-sourced fields — the same placeDetailsAreCurrent rule the nearby-search path + // trusts. restoreCachedDetails below copies those fields onto the confirmed place, so the + // skip loses nothing; re-confirming a place we already hold must not re-buy its details. + if stored, ok := cached[placeID]; ok && placeDetailsAreCurrent(stored, time.Now()) { + Logger.Debugf("addSearchedPlaceToCache: skipping Place Details for %s — cached record is current", placeID) } else { - foldPlaceDetailsIntoPlace(&place, details) + details, enrichErr := enrich(ctx, placeID) + if enrichErr != nil { + Logger.Errorf("addSearchedPlaceToCache: Place Details enrich failed for %s, continuing with the lean stashed record: %v", placeID, enrichErr) + } else { + foldPlaceDetailsIntoPlace(&place, details) + } } places := []POI.Place{place} diff --git a/planner/planner.go b/planner/planner.go index c8d1818c5..17b91379c 100644 --- a/planner/planner.go +++ b/planner/planner.go @@ -232,7 +232,7 @@ func (p *MyPlanner) Destroy() { func (p *MyPlanner) reverseGeocodingHandler(ctx *gin.Context) { latitude, _ := strconv.ParseFloat(ctx.Query("lat"), 64) longitude, _ := strconv.ParseFloat(ctx.Query("lng"), 64) - result, err := p.Solver.Searcher.GetMapsClient().ReverseGeocode(ctx, latitude, longitude) + result, err := p.Solver.Searcher.ReverseGeocode(ctx, latitude, longitude) if err != nil { log.Error(err) ctx.JSON(http.StatusInternalServerError, err.Error()) From b7c8ff3762de8009551d5f22fab0536023b73714 Mon Sep 17 00:00:00 2001 From: tim-eternos Date: Sat, 1 Aug 2026 15:35:00 -0700 Subject: [PATCH 2/2] Replace BusinessStatus request field with fail-safe IncludeClosedPlaces flag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that closures are persisted in the cache, the old contract was one forgotten field away from breaking planners: filtering only happened when a caller remembered BusinessStatus: POI.Operational, so a zero-value PlaceSearchRequest would serve permanently-closed places from both cache reads and cold-search responses. The zero value is now safe: non-Operational places are always filtered unless the caller explicitly opts in with IncludeClosedPlaces. All three production call sites (matcher, both planner handlers) already requested Operational filtering, so their behavior is unchanged — the field they set is simply gone. The RemovePlaces migration test opts in: it verifies raw bucket contents and its fixtures carry no Status. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01DjdGnZM6cUBeg6jWwoQfU3 --- iowrappers/closure_persistence_test.go | 60 +++++++++++++------ iowrappers/data_migrations_test.go | 7 ++- iowrappers/nearby_search.go | 8 ++- iowrappers/poi_searcher.go | 2 +- iowrappers/redis_client.go | 2 +- matching/matcher.go | 1 - planner/planner.go | 2 - test/redis_client_mocks/nearby_search_test.go | 1 - 8 files changed, 57 insertions(+), 26 deletions(-) diff --git a/iowrappers/closure_persistence_test.go b/iowrappers/closure_persistence_test.go index 82d03b35a..3a4875c35 100644 --- a/iowrappers/closure_persistence_test.go +++ b/iowrappers/closure_persistence_test.go @@ -11,6 +11,11 @@ import ( // discarded the closure signal entirely: the stale record kept Status OPERATIONAL forever and the // place kept being served from cache. Persisting the closure lets the read-side Operational // filters (RedisClient.NearbySearch) retire the place everywhere, at zero extra API cost. +// +// Filtering is the DEFAULT: now that closed places genuinely live in the cache, a caller that +// forgets to ask for filtering must never receive them — planners consume these results and a +// zero-value request has to be safe. IncludeClosedPlaces is the explicit opt-in for the rare +// caller that wants everything. func TestColdSearchPersistsClosuresButExcludesThemFromResponse(t *testing.T) { s, ctx := newAddSearchedPlaceFixture(t) @@ -26,18 +31,18 @@ func TestColdSearchPersistsClosuresButExcludesThemFromResponse(t *testing.T) { Name: "Shuttered Grill", LocationType: POI.LocationType("restaurant"), Status: POI.ClosedPermanently, - Location: POI.Location{Latitude: 37.41, Longitude: -122.11}, + Location: POI.Location{Latitude: 37.401, Longitude: -122.101}, } + // Deliberately a zero-value request apart from the search identity: the default must filter. req := &PlaceSearchRequest{ - PlaceCat: POI.PlaceCategoryEatery, - BusinessStatus: POI.Operational, - Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + PlaceCat: POI.PlaceCategoryEatery, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, } got := s.persistAndFilterSearchResults(ctx, req, []POI.Place{open, closed}) if len(got) != 1 || got[0].ID != "open-1" { - t.Fatalf("response = %+v, want only open-1 (closed places excluded from the response)", got) + t.Fatalf("response = %+v, want only open-1 (closed places excluded by default)", got) } cached, err := s.redisClient.CachedPlaces(ctx, []string{"open-1", "closed-1"}) @@ -55,13 +60,12 @@ func TestColdSearchPersistsClosuresButExcludesThemFromResponse(t *testing.T) { t.Errorf("closed-1 Status = %q, want %q recorded", stored.Status, POI.ClosedPermanently) } - // The read path must retire it: an Operational-filtered cache read excludes the closed place. + // The read path must retire it BY DEFAULT: a zero-value cache read excludes the closed place. readReq := &PlaceSearchRequest{ - PlaceCat: POI.PlaceCategoryEatery, - BusinessStatus: POI.Operational, - Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, - Radius: 1000, - MinNumResults: 1, + PlaceCat: POI.PlaceCategoryEatery, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + Radius: 1000, + MinNumResults: 1, } fromCache, err := s.redisClient.NearbySearch(ctx, readReq) if err != nil { @@ -69,25 +73,47 @@ func TestColdSearchPersistsClosuresButExcludesThemFromResponse(t *testing.T) { } for _, p := range fromCache { if p.ID == "closed-1" { - t.Error("closed-1 served from an Operational-filtered cache read") + t.Error("closed-1 served from a default cache read — planners would receive a closed place") } } } -func TestPersistAndFilterWithoutStatusFilterReturnsEverything(t *testing.T) { +func TestIncludeClosedPlacesOptsIntoUnfilteredResults(t *testing.T) { s, ctx := newAddSearchedPlaceFixture(t) places := []POI.Place{ {ID: "a", Name: "A", LocationType: POI.LocationType("restaurant"), Status: POI.Operational, Location: POI.Location{Latitude: 37.4, Longitude: -122.1}}, - {ID: "b", Name: "B", LocationType: POI.LocationType("restaurant"), Status: POI.ClosedPermanently, Location: POI.Location{Latitude: 37.41, Longitude: -122.11}}, + {ID: "b", Name: "B", LocationType: POI.LocationType("restaurant"), Status: POI.ClosedPermanently, Location: POI.Location{Latitude: 37.401, Longitude: -122.101}}, } req := &PlaceSearchRequest{ - PlaceCat: POI.PlaceCategoryEatery, - Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + PlaceCat: POI.PlaceCategoryEatery, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + IncludeClosedPlaces: true, } got := s.persistAndFilterSearchResults(ctx, req, places) if len(got) != 2 { - t.Fatalf("response has %d places, want 2 — no BusinessStatus filter was requested", len(got)) + t.Fatalf("response has %d places, want 2 — IncludeClosedPlaces opted into everything", len(got)) + } + + readReq := &PlaceSearchRequest{ + PlaceCat: POI.PlaceCategoryEatery, + Location: POI.Location{Latitude: 37.4, Longitude: -122.1}, + Radius: 1000, + MinNumResults: 1, + IncludeClosedPlaces: true, + } + fromCache, err := s.redisClient.NearbySearch(ctx, readReq) + if err != nil { + t.Fatalf("RedisClient.NearbySearch: %v", err) + } + foundClosed := false + for _, p := range fromCache { + if p.ID == "b" { + foundClosed = true + } + } + if !foundClosed { + t.Error("IncludeClosedPlaces read did not return the closed place") } } diff --git a/iowrappers/data_migrations_test.go b/iowrappers/data_migrations_test.go index 5a3a1b3fd..c51a8270a 100644 --- a/iowrappers/data_migrations_test.go +++ b/iowrappers/data_migrations_test.go @@ -72,7 +72,8 @@ func TestRemovePlaces(t *testing.T) { var err error redisClient.SetPlacesAddGeoLocations(ctx, []POI.Place{placeA, placeB, placeC}) places, _ = redisClient.NearbySearch(ctx, &PlaceSearchRequest{ - PlaceCat: POI.PlaceCategoryVisit, + PlaceCat: POI.PlaceCategoryVisit, + IncludeClosedPlaces: true, // migration test reads raw bucket contents; fixtures carry no Status Location: POI.Location{ Latitude: 12.5636, Longitude: 14.7813, @@ -92,7 +93,8 @@ func TestRemovePlaces(t *testing.T) { } places, _ = redisClient.NearbySearch(ctx, &PlaceSearchRequest{ - PlaceCat: POI.PlaceCategoryVisit, + PlaceCat: POI.PlaceCategoryVisit, + IncludeClosedPlaces: true, // migration test reads raw bucket contents; fixtures carry no Status Location: POI.Location{ Latitude: 12.5636, Longitude: 14.7813, @@ -106,6 +108,7 @@ func TestRemovePlaces(t *testing.T) { } places, _ = redisClient.NearbySearch(ctx, &PlaceSearchRequest{PlaceCat: POI.PlaceCategoryEatery, + IncludeClosedPlaces: true, // migration test reads raw bucket contents; fixtures carry no Status Location: POI.Location{ Latitude: 12.5636, Longitude: 14.7813, diff --git a/iowrappers/nearby_search.go b/iowrappers/nearby_search.go index 9f04632a0..8c6194468 100644 --- a/iowrappers/nearby_search.go +++ b/iowrappers/nearby_search.go @@ -35,7 +35,13 @@ type PlaceSearchRequest struct { // suppose a location has more places established over time, this field would help trigger new searches to get those new establishments. MinNumResults uint - BusinessStatus POI.BusinessStatus + // IncludeClosedPlaces opts a caller INTO receiving non-Operational places. The zero value + // filters them from both cache reads and cold-search responses: closed places are persisted + // in the cache (their closure is the signal that retires them), so a request that forgot to + // ask for filtering must never serve one — planners consume these results directly. This + // replaces the old BusinessStatus field, whose zero value was the UNFILTERED behavior and + // therefore one forgotten assignment away from leaking closures. + IncludeClosedPlaces bool // true if using precise geolocation instead of using a grander administrative area UsePreciseLocation bool diff --git a/iowrappers/poi_searcher.go b/iowrappers/poi_searcher.go index fcc72dc28..a67e2c020 100644 --- a/iowrappers/poi_searcher.go +++ b/iowrappers/poi_searcher.go @@ -288,7 +288,7 @@ func (s *PoiSearcher) persistAndFilterSearchResults(ctx context.Context, req *Pl } } - if req.BusinessStatus == POI.Operational { + if !req.IncludeClosedPlaces { totalPlacesCount := len(newPlaces) newPlaces = Filter(newPlaces, func(place POI.Place) bool { return place.Status == POI.Operational }) Logger.Debugf("%d places out of %d left after business status filtering", len(newPlaces), totalPlacesCount) diff --git a/iowrappers/redis_client.go b/iowrappers/redis_client.go index ab7cb2969..124f7b834 100644 --- a/iowrappers/redis_client.go +++ b/iowrappers/redis_client.go @@ -581,7 +581,7 @@ func (r *RedisClient) NearbySearch(ctx context.Context, req *PlaceSearchRequest) ctx.Value(ContextRequestIdKey), redisKey, orphans) } - if req.BusinessStatus == POI.Operational { + if !req.IncludeClosedPlaces { totalPlacesCount := len(places) places = Filter(places, func(place POI.Place) bool { return place.Status == POI.Operational }) Logger.Debugf("(RedisClient)NearbySearch -> %d places out of %d left after business status filtering", len(places), totalPlacesCount) diff --git a/matching/matcher.go b/matching/matcher.go index 61bc75212..2623a05e6 100644 --- a/matching/matcher.go +++ b/matching/matcher.go @@ -79,7 +79,6 @@ func NearbySearchForCategory(ctx context.Context, searcher iowrappers.SearchClie Location: req.Location, Radius: req.Radius, MinNumResults: MinResultsForTimePeriodMatching, - BusinessStatus: POI.Operational, UsePreciseLocation: req.UsePreciseLocation, PriceLevel: req.PriceLevel, } diff --git a/planner/planner.go b/planner/planner.go index 17b91379c..3f24ff6f7 100644 --- a/planner/planner.go +++ b/planner/planner.go @@ -1312,7 +1312,6 @@ func (p *MyPlanner) getNearbyPlaces(ctx *gin.Context) { Radius: radius, MinNumResults: uint(limit), DetailsLimit: limit, - BusinessStatus: POI.Operational, } result := nearbyPlacesBrandResult{Brand: keyword, Places: []POI.Place{}} places, searchErr := p.Solver.Searcher.NearbySearch(searchContext, searchReq) @@ -1441,7 +1440,6 @@ func (p *MyPlanner) getNearbyPlacesByCategory(ctx *gin.Context) { // Bound the expensive Place Details calls even when the candidate // pool (limit) is widened for dedup — details cost stays ~today's. DetailsLimit: min(limit, 20), - BusinessStatus: POI.Operational, } result := nearbyPlacesByCategoryResult{Category: string(placeCat), Places: []POI.Place{}} places, searchErr := p.Solver.Searcher.NearbySearch(searchContext, searchReq) diff --git a/test/redis_client_mocks/nearby_search_test.go b/test/redis_client_mocks/nearby_search_test.go index 59785ce6a..24f0f217b 100644 --- a/test/redis_client_mocks/nearby_search_test.go +++ b/test/redis_client_mocks/nearby_search_test.go @@ -152,7 +152,6 @@ func TestGetPlaces_shouldExcludePlacesNotOperational(t *testing.T) { Location: POI.Location{Longitude: -74.0060, Latitude: 40.7128}, PlaceCat: POI.PlaceCategoryVisit, Radius: uint(20000), - BusinessStatus: POI.Operational, } cachedVisitPlaces, _ := RedisClient.NearbySearch(RedisContext, &placeSearchRequest)