fix: stop legacy Places API writing mislabeled places into category buckets

[timwangmusic]

Description

Fixes a production data-corruption bug introduced by #446, plus two independent bugs found while investigating it.

The incident. POI.GetPlaceCategory had a default: branch returning PlaceCategoryEatery, silently absorbing any place type the legacy Places API does not understand. That default also made TestPlaceCategoryRoundTrip un-failable, so #446 was able to add two Places-API-(New)-only types — fast_food_restaurant and food_court — to GetPlaceTypes(Eatery) with a green suite. Neither string exists in googlemaps.github.io/maps@v1.7.0.

The legacy Nearby Search does not reject an unknown ?type=. It ignores the filter and returns prominence-ranked establishments. parsePlacesSearchResponse then stamps the queried type onto every result, so hotels were written into placeIDs:eatery:level* carrying LocationType: fast_food_restaurant.

Measured blast radius. A read-only dry run against production on 2026-07-30 (24,203 bucket members scanned) found 148 incident-stamped records, not the ~17 first estimated from a single city's buckets — mostly San Francisco, Tulsa and Boise hotels. Full numbers in docs/migrations/reclassify-buckets.md.

These were user-visible, not just cache residue. POI.ReclassifyForCategory hides them from the merchant endpoint, which is why the API looked clean. But it has exactly one caller (planner/planner.go). The trip-planning path — planner/solver.go → matching.NearbySearchForCategory → matching.CreatePlace — reads the same buckets and never reclassifies, so the hotels were being slotted into generated trip plans as eateries.

Correctly classifying fast food is not possible on the legacy API at all: its response types[] never contains those values, so nothing downstream can assign them either. That needs Places API (New) searchNearby with includedPrimaryTypes — see follow-ups below.

Solution

1. Make unknown place types un-mappable (a2ded84)
GetPlaceCategory now returns (PlaceCategory, bool) — the shape ParsePlaceCategory already used — with no default category, so the compiler forces every call site to decide what an unmapped type means. The two Redis write paths refuse to guess a bucket and log instead. The saved-plan display path deliberately keeps the historical Eatery fallback, so that endpoint's output is byte-identical — the asymmetry is intentional, not an oversight. The two constants are removed.

This is what makes the guard real: re-adding LocationTypeFastFood now fails 3 tests across 2 packages.

2. Reject non-legacy place types before spending an API call (98f940b, c1853f1)
CreateMapSearchRequest cast POI.LocationType straight to maps.PlaceType, bypassing the SDK's own ParsePlaceType validator. It now validates and fails loudly. POI.LocationTypeAny (brand searches, type deliberately unset) still passes.

Also repairs a dead retry cap: maxRetries was computed as reqTimes * len(placeTypes) while reqTimes was still 0, so the cap was always 0 and break outer unreachable. Making it live exposed that the shared cross-round failure counter let one flaky place type exhaust a whole category's budget and silently truncate its healthy siblings, so the counter now resets per round.

3. Sort by distance before truncating (92d615b) — independent pre-existing bug
places[:limit] assumed distance ordering, which only holds on the Redis cache path. The fresh path appends each place type's page in Google prominence order, so a cold search kept roughly cafes and restaurants and dropped bar, bakery and meal_takeaway entirely — the exact types #445 added. It could also rank a 3km result above one 250m away.

⚠️ This changes result ordering on the live cold path of /v1/nearby-places-by-category from prominence to distance. Intended, but it is a product-visible change riding inside a bugfix. Rollback is the single SortPlacesByDistance call in planner/planner.go.

4. Cleanup migration (299a8fd, 267e690)
GET /v1/migrate/reclassify-buckets?category=Eatery, admin-gated, dry-run unless apply=true. Runbook: docs/migrations/reclassify-buckets.md.

The cleanup rule is the exact inverse of the write rule: it removes a member only when its primary Google type positively maps to a different category. lodging → Lodging and supermarket → Shopping are removed; meal_delivery, night_club and records with no types[] are kept, because the write path would legitimately place them there and would immediately re-create them.

⚠️ The two rules now deliberately disagree. A future refactor that "unifies" them reintroduces the incident. The dangerous direction is pinned by TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable.

Reads are pipelined in batches of 100 and the report carries bucket_sizes/total_members, because a serial N+1 scan cannot finish inside Heroku's hard 30s H12 — which would have defeated the review-before-apply safety property. Verified against prod: full 8,589-member Eatery scan in ~1.3s.

Deployment

Deploy this before running the migration — 8644199 is still creating new bad records on every cold search past the 14-day marker, so cleaning first just gets re-polluted. Then dry-run, read the output, then apply=true. Steps and measured expectations are in the runbook.

Follow-ups (not in this PR)

• POI.AllPlaceCategories — highest value. Both guard tests enumerate categories by hand, so they protect today's five categories rather than the invariant. Adding a new category with bogus place types still reproduces this incident with a fully green suite.
• getNearbyPlacesByBrand (planner/planner.go) still truncates by prominence under the comment this PR falsified. Ordering-only impact.
• 27-record residue — incident records whose primary type maps to no category (university, airport, stadium, real_estate_agency, …) are kept by the cleanup rule and remain reachable in trip plans. Broadening GetPlaceCategory would make them removable.
• ~328 orphaned bucket members with no backing place_details record. Pre-existing, unrelated.
• Places API (New) migration — the real fix for fast-food classification.

Full implementation plans and the detailed follow-up list are preserved on origin/docs/place-type-plans rather than carried in this PR; they were 2906 of the original 3902 added lines.

Testing

• Integration testing on Heroku staging — not done, needs a deploy plus a GOOGLE_MAPS_API_KEY
• Added new unit tests

673 added test lines across 5 files (2 new). go build -v . clean, go test ./... 6/6 packages ok, go vet clean. The migration was additionally exercised read-only against production and against a seeded reproduction of the incident records.

Checks

• Have you removed commented code?
• Have you used gofmt to format your code? (gofmt -l empty)

[tim-eternos]

Post-merge review — flagging this as follow-up material rather than merge-blocking, since it's already in master. Nothing below would have blocked; the core fix is sound.

Overview

Three related changes plus a cleanup migration:

1. Root-cause fix — fast_food_restaurant and food_court (Places API (New)-only types) removed from GetPlaceTypes(Eatery), and GetPlaceCategory loses its default: Eatery branch in favour of (PlaceCategory, bool). The default was the actual bug: it made TestPlaceCategoryRoundTrip structurally incapable of failing, which is why two invalid types could be added without any test noticing.
2. Boundary validation — CreateMapSearchRequest now returns an error for any type maps.ParsePlaceType doesn't recognise, so an unenforceable ?type= fails before the API spend rather than after the cache is poisoned.
3. Write-path guard — SetPlacesAddGeoLocations and StorePlacesForLocation refuse to file a place whose type maps to no category.
4. Cleanup migration — GET /v1/migrate/reclassify-buckets, admin-only, dry-run by default.

Two things worth calling out as good practice, because they're the reason this review is short:

• The comments explain the failure mode, not the code. GetPlaceCategory's doc comment states why there is no default and what happened when there was one. The maxRetries comment says "it must NOT be hoisted out here" and explains the consequence. Six months from now these are what stop someone re-introducing the bug.
• docs/migrations/reclassify-buckets.md reports a real dry run — 24,203 members scanned, 164 candidates, and, critically, the 27 records the rule doesn't catch, named by type. Documenting your own fix's residue is rarer than it should be. Replacing 2,906 lines of planning docs with this was the right call.

The mapsFailuresCount fix is also a real latent bug caught in passing: maxRetries := reqTimes * uint(len(placeTypes)) evaluated while reqTimes == 0 made the cap zero, so if mapsFailuresCount == maxRetries never fired and the bail-out was dead code. Moving to a per-round counter with >= is correct on both axes.

---

Follow-ups worth filing

1. The guard tests still can't fail for a new category

Highest-value item, because it's the same shape as the original incident. Both guards hand-enumerate today's categories:

// test/place_category_test.go — TestPlaceCategoryRoundTrip
categories := []POI.PlaceCategory{
    POI.PlaceCategoryVisit, POI.PlaceCategoryEatery,
    POI.PlaceCategoryShopping, POI.PlaceCategoryLodging, POI.PlaceCategoryWellness,
}

// iowrappers/nearby_search_validation_test.go — TestCreateMapSearchRequestAcceptsKnownPlaceTypes
categories := []POI.PlaceCategory{ /* the same five, again */ }

So they protect the five categories that exist today, not the invariant. Add a PlaceCategoryNightlife whose GetPlaceTypes returns {"karaoke", "pub"} — neither is a legal legacy type, exactly the fast_food_restaurant mistake — plus a matching GetPlaceCategory case, and the suite stays green. The incident is reproducible verbatim for any category added from here.

Suggested fix: add POI.AllPlaceCategories and drive ParsePlaceCategory, TestPlaceCategoryRoundTrip, TestCreateMapSearchRequestAcceptsKnownPlaceTypes, and the migration handler off it. A new category then lands inside every guard automatically. That turns "we fixed this bug" into "this bug shape can't come back", which is the actual goal.

2. The brand handler carries the comment this PR just falsified

getNearbyPlacesByCategory was correctly fixed. Its sibling getNearbyPlaces still reads:

places = iowrappers.Filter(places, func(place POI.Place) bool { return !place.KnownClosedOnDay(day) })
// Redis results are sorted by distance ascending; keep the nearest ones
if len(places) > limit {
    places = places[:limit]
}

True on the cache path, false on the fresh path — PoiSearcher.NearbySearch returns newPlaces in Google prominence order. A cold brand search can keep a 5km Dunkin' and drop a 300m one. Impact is ordering-only (brand searches use a single LocationTypeAny type, so no whole place types are lost), which is presumably why it was left — but the repo now has one handler sorted and its neighbour unsorted, carrying a comment the new SortPlacesByDistance doc comment explicitly refutes. One line: iowrappers.SortPlacesByDistance(places, req.Location.Latitude, req.Location.Longitude) before the truncation.

3. The write guard drops the place-details record too, not just the geo write

placeCategory, ok := POI.GetPlaceCategory(place.LocationType)
if !ok {
    Logger.Errorf("... skipping geo bucket write", ...)
    continue          // <- also skips pipe.Set(PlaceDetailsRedisKeyPrefix+place.ID, ...)
}

The continue exits the whole loop body, so the place_details:place_ID:* record is skipped as well, and the log line says only "skipping geo bucket write". The details record is keyed by place ID, not by category — it can't poison a bucket — and dropping it means a later getPlace for that ID misses for no benefit. Suggest writing the details record and skipping only the GeoAdd, or updating the message to say both are skipped.

(Reachability is currently low: post-validation parsePlacesSearchResponse only stamps types that passed ParsePlaceType, and brand searches go through SetPlacesAddGeoLocationsForBrand instead. So this is mostly about the guard behaving as documented if it ever does fire.)

4. AddGeoLocation opens a hole in the invariant this PR establishes

// AddGeoLocation adds a place to a geo bucket under an explicit key. Exported for
// migrations and tests that need to write buckets the normal write path would reject.
func (r *RedisClient) AddGeoLocation(ctx context.Context, key string, place POI.Place) error {

This is a new exported method on the production RedisClient whose stated purpose is to bypass the type validation the rest of the PR just added, and it has no production caller — it exists so tests can seed bad buckets. Worth either moving it behind a test-only build tag / export_test.go, or having it take a POI.PlaceCategory and derive the key so it can't write an arbitrary one.

Minor, same area: SetPlace and AddGeoLocation are generic RedisClient concerns living in data_migrations.go; redis_client.go is their natural home.

5. A Redis fault mid-scan reports as "buckets are clean"

for i, placeID := range batch {
    report.Scanned++
    if !found[i] {
        Logger.Debugf("... no record for %s in %s", placeID, key)
        continue
    }

getPlacesPipelined collapses "key absent" (redis.Nil) and "read failed" into the same found[i] = false. A transport fault partway through therefore skips every remaining member and returns Misclassified: 0, which an operator will reasonably read as "nothing to clean". Fail-safe in direction — nothing gets deleted — but actively misleading. This matters more than it looks because the runbook records ~328 members with no backing record, so the skip path fires on every real run and its count is invisible.

Suggest distinguishing redis.Nil from other command errors, and adding skipped / read_errors counts to BucketCleanupReport so Scanned reconciles.

6. RemovedIDs is populated during a dry run

report.Misclassified++
report.RemovedIDs = append(report.RemovedIDs, placeID)
...
if dryRun {
    continue
}

In a dry run removed_ids lists things that were not removed, while removed is 0. The response does echo dry_run, so it's recoverable, but for a report whose entire job is to be read carefully before deleting production data, candidate_ids (or would_remove_ids) would be worth the rename.

7. Migration scalability

All irrelevant at the measured 164 rows; filing for completeness:

• ZRem is not pipelined. Reads were batched into pipelines of 100 specifically because a serial N+1 can't finish inside Heroku's 30s H12, but removals are still one round trip per member — so the stated rationale now covers only the read half.
• BucketSizes / TotalMembers are measured up front but only serialised in the terminal ctx.JSON. An H12 severs the request, so the operator who most needs the scale figure is exactly the one who never receives it (partial_report covers returned errors, not a router timeout). A ?sizes=true mode returning straight after the ZCARD loop would make that unconditional.
• ZRange(key, 0, -1) pulls all members into memory at once (14,674 for Visit). Fine at current scale; ZScan is the scalable form.

8. Small stuff

• SortPlacesByDistance uses map[int]float64 for dense integer keys — a []float64 avoids the hashing and the allocation. The precompute-then-sort-indices approach is right (keeps Haversine at O(n) rather than O(n log n)), and the "sort is stable so equal distances keep their existing relative order" comment is accurate here, since idx starts in slice order.
• RemoveMisclassifiedPlacesFromCategoryBuckets relies on EncodeNearbySearchRedisKey ignoring the price level for non-Eatery categories to make levels := []POI.PriceLevel{POI.PriceLevelDefault} correct. That coupling is load-bearing and only documented over on POI.AllPriceLevels; a one-line comment at the levels assignment would help.
• The handler doc comment in planner/planner.go says the migration "removes places from a category's geo buckets whose primary Google type does not belong to that category" — that's the broader rule that was deliberately not implemented. The runbook and the RemoveMisclassifiedPlacesFromCategoryBuckets doc both correctly describe the narrower positively-maps-elsewhere rule. Since these rules intentionally disagree with the write rule, the comments are what hold them apart, so this one is worth aligning.
• iowrappers/nearby_search.go: the failure log says "places nearby search with Maps failed for place type %s" even when the cause is a CreateMapSearchRequest validation rejection and no search was attempted. Cheap fix, and worth it because "fail loudly and accurately" is that code's whole point.

---

Residual risk worth keeping visible

The write rule and the cleanup rule now legitimately disagree — the write path keys on the stamped LocationType, the cleanup keys on Google's primary type and deliberately keeps unmapped primaries (meal_delivery, night_club, empty types[]) because the write path would legitimately put them there. The runbook says this and TestRemoveMisclassifiedPlacesPrimaryTypeTruthTable pins the dangerous direction, which is the right defence.

What isn't pinned is someone changing POI.ReclassifyForCategory itself, or collapsing both rules into one shared helper "for consistency". That's why item 8's comment-accuracy point is load-bearing rather than cosmetic.

Also worth not relearning: ReclassifyForCategory has exactly one production caller (the merchant endpoint). The trip-planning path — planner/solver.go → matching.NearbySearchForCategory → matching.CreatePlace — reads the same placeIDs:eatery:level* buckets and never reclassifies. So the 27 residue records the runbook documents stay reachable in generated plans. Anything reasoning about "what the buckets contain" has to account for both readers.

---

Reviewed by reading the diff against b8b643f; no Go toolchain was available in my environment, so treat CI as authoritative on anything mechanical.