Do not index deleted forts loaded from the database

[TurtIeSocks]

This PR started as an in-memory rewrite of the geofence endpoints and has been reduced to the one bug it turned up along the way. The timeout it was chasing is fixed in #400 instead, on the database path, so nothing here depends on fort_in_memory.

Problem

fortRtreeUpdatePokestopOnGet and fortRtreeUpdateGymOnGet index every fort they are handed. The load-by-id queries carry no deleted filter, and deleting a fort leaves enabled and the quest fields intact, so a deleted row arriving through the cache-miss path was added to the lookup cache and the fort tree as a live fort. /api/pokestop/scan and /api/gym/scan would then return it.

The save path (fortRtreeUpdatePokestopOnSave into genericUpdateFort) has always skipped deleted forts. The load path now matches it.

Why the rest went away

The original approach walked the fort lookup index instead of querying the database, and measurement did not support it. Against 1,000,000 rows and a 2000-vertex fence, the in-memory walk and the database path spend most of their time in the same place: testing candidate points against the polygon. Moving that test out of SQL takes the query from 55 s to roughly 4 s, and the in-memory version saves only the second or so of query and transfer on top of that. The numbers are in #400.

That is not worth what it cost. Serving these endpoints from the index meant treating it as a census of the table, which it was not: the fort caches expire entries untouched for 25 to 27 hours and preload only runs at boot, so a region that stopped being scanned quietly dropped out of the answers. Keeping it complete meant holding every fort's index entry for the process lifetime, and the invariant had no test behind it. All of that to avoid a second of query time, on instances with fort_in_memory enabled and nowhere else.

@jfberry's read was right, and the database is the correct place for this. #400 keeps it there.

Testing

TestDeletedFortsNotIndexedOnLoad covers a deleted pokestop, a deleted gym, and a live pokestop that must still be indexed so the guard cannot pass by rejecting everything. It fails with either guard removed.

go build -tags go_json ./... and golangci-lint run are clean, and the decoder and root suites pass.

🤖 Generated with Claude Code

[Mygod]

The patch treats an evicting and not strictly deletion-safe cache as authoritative for formerly DB-backed endpoints, yielding silent omissions and inclusions. Its hot walker also adds per-match allocations and multipolygon-wide containment work that can defeat the stated large-geofence performance goal.

Full review comments:

• [P1] Require a complete index before bypassing SQL — decoder/fort_fence_scan.go:15-15
  With fort_in_memory on, this remains true even after a pokestop's 25–27 h cache TTL expires; the eviction callback then removes its lookup/tree entry (CLAUDE.md:152 and CLAUDE.md:158). Consequently a long-idle region is silently omitted from /api/pokestop-positions and /api/quest-status, and /api/clear-quests leaves those DB rows untouched, whereas the previous SQL path still covered them. Gate on a real completeness signal/use a non-evicting index, or retain SQL for these whole-DB operations.

• [P1] Avoid allocating a FortLookup for every match — decoder/fort_fence_scan.go:89-89
  For the country-sized scans this path is meant to make practical (CLAUDE.md:403-407), taking &lookup and passing it to an indirect callback forces the large local copy to escape to the heap once per matched pokestop. A million-stop status request therefore creates a million extra heap objects and substantial GC/memory pressure, potentially replacing the SQL timeout with process stalls; pass the value or only the required scalar fields instead.

• [P2] Check only the polygon that produced each candidate — decoder/fort_fence_scan.go:84-84
  When a MultiPolygon has many disjoint parts, this call is already inside the search for one part's bound, but CompiledFence.Contains starts at polygon 0 and scans every part again. Candidates distributed across P islands thus require O(candidates × P) bound checks, undermining the per-polygon optimization described in CLAUDE.md:405; expose/use containment for the current polygon index, then keep the existing ID dedupe for overlaps.

• [P2] Filter rehydrated deleted pokestops — decoder/fort_fence_scan.go:81-81
  When a deleted pokestop is loaded after its entity-cache entry expires, for example through the nearby-Pokémon or webhook read paths, fortRtreeUpdatePokestopOnGet indexes it unconditionally even though Deleted is true; deletion itself leaves Enabled and quest fields intact. This type-only check then includes that row, so the new in-memory /api/quest-status can violate the former deleted = 0 predicate and the invariant claimed in CLAUDE.md:405; carry/filter the deleted bit or skip indexing deleted loads.

[jfberry]

It was a design choice to do the geofence in the database for this - since we don’t nescessarily have all the records in memory.

Can the query not just be optimised to avoid the particular issue?

[jfberry]

A quick google suggests these variations are worth trying in the first instance:

SET @Fence = ST_GeomFromGeoJSON(?, 2, 0);

SELECT ...
FROM places
WHERE lat > ?
AND lon > ?
AND lat < ?
AND lon < ?
AND enabled = 1
AND deleted = 0
AND ST_Contains(@Fence, POINT(lon, lat));

or

WITH candidates AS MATERIALIZED (
SELECT id, lon, lat
FROM places
WHERE enabled = 1
AND deleted = 0
AND lat BETWEEN ? AND ?
AND lon BETWEEN ? AND ?
)
SELECT ...
FROM candidates
WHERE ST_Contains(@Fence, POINT(lon, lat));

[TurtIeSocks]

Thanks both. This ended up a lot smaller than it started.

@Mygod all four were real, and I checked each rather than taking them on faith:

• Index completeness. Correct, and it's why the in-memory path is gone entirely instead of patched. Fort caches expire at 25-27h and preload only runs at boot, so quiet regions were quietly dropping out of the answers.
• The &lookup escape. Confirmed with go build -gcflags=-m, which reported moved to heap: lookup for every match.
• Per-polygon containment. Valid, though the cost was P bound checks per candidate rather than P ray casts, since Contains does a cheap bounds test before any real work.
• Deleted forts on the load path. Real, and pre-existing. It also hits /api/pokestop/scan and /api/gym/scan, which could hand back a deleted fort. That one fix is all that's left here.

@jfberry you were right, and I had the cause wrong. I benchmarked both suggestions on MariaDB 11.8, 1M rows, a 2000 vertex fence, 628,560 bbox candidates:

variant	time
current query, fence inlined	55.0 s
SET @fence then ST_CONTAINS(@fence, ...)	57.6 s
the same through a CTE	57.8 s
WITH ... AS MATERIALIZED	syntax error on MariaDB
bbox scan alone, all rows read	0.9 s
those same 628,560 points in Go	3.3 s

Neither variant moves it, and AS MATERIALIZED turns out to be Postgres syntax. But the parse hoist changing NOTHING is the useful result, because it rules out repeated parsing and leaves per row ST_CONTAINS as the cost. So the fix is to stop asking SQL for containment at all, which is #400. That keeps it in the database path with no fort_in_memory dependency, which is what you were asking for.

Synthetic data in a container, so please read the ratio rather than the absolute times.