Skip to content

Request-first lure pokemon processing - #391

Merged
jfberry merged 8 commits into
mainfrom
fix/lure-request-first
Jul 29, 2026
Merged

Request-first lure pokemon processing#391
jfberry merged 8 commits into
mainfrom
fix/lure-request-first

Conversation

@jfberry

@jfberry jfberry commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Fixes #390. Closes #283.

Problem

A failed pokestop lookup while processing a lure MapPokemonProto committed a partially initialized pokemon record: indexed in the R-tree at (0,0), no pokestop_id, no expiry, marked non-new — and therefore permanently unrepairable, while a later DiskEncounter would happily attach IVs and fire hundo webhooks at lat/lon 0 with disappear_time 0 (#390). Structurally, the whole diskEncounterCache existed only because the DiskEncounter response carries no location, forcing responses to wait for a GMO (#283).

Approach

Each proto contributes what it alone knows, in any arrival order:

Source Contributes
GMO fort entry (fort.ActivePokemon) placement (fort id + coords, captured at extraction) and the verified despawn time
DiskEncounterProto (request) placement for encounter-first arrivals (encounter id, fort id, fort coords)
DiskEncounterOutProto (response) species, IVs, CP, level
  • No lure path touches the pokestop record anymore. The lure pokemon rides inside its fort's own GMO entry, so RawMapPokemonData now carries the fort's id/lat/lon captured at extraction — placement cannot fail, even in pokemon-only scan contexts where the fort cache is never populated.
  • decodeDiskEncounter now requires and parses the request proto (same posture as GetStationDetails): encounter id comes from request.EncounterId (the PokemonDisplay.DisplayId stand-in is gone), and an encounter with no prior GMO creates a fully placed lure_encounter record with a 180 s unverified expiry (lure spawns live 3 minutes). A later GMO tightens it to the verified despawn. Requests missing the payload are counted (request_missing) and skipped.
  • updateFromMap is now an order-tolerant merge: new records get placed from the captured fort fields; existing lure records only receive GMO-owned facts (verified expiry, cell id, username backfill) — never a seen-type downgrade, never touching encounter data, and no-change sightings skip the save.
  • diskEncounterCache is deleted — nothing waits for anything.

Testing

  • Regression tests mirror the Lure Pokemon becomes unqueryable after Pokestop lookup failure #390 repro end-to-end: unknown-fort GMO placement, encounter-first creation (webhook carries real coords and a future expiry), GMO-after-encounter merge, GMO-first upgrade, request-missing skip, and v2/v3 scan visibility of estimated-expiry records.
  • go build, go vet, full go test ./..., and golangci-lint run are clean; the new tests pass under -race.
  • Note: a pre-existing (on main) test race in decoder/station_battle_test.go (direct statsCollector reassignment vs the async stats worker) was found while verifying — left for a follow-up since it's unrelated to this branch.

🤖 Generated with Claude Code

jfberry and others added 7 commits July 29, 2026 18:56
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A MapPokemonProto only ever arrives as fort.ActivePokemon inside its
fort's own GMO entry, so capture the fort id and coordinates at
extraction instead of re-deriving them from a pokestop lookup that can
miss (and whose failure used to commit an unplaced record at 0,0 that
no later GMO could repair). updateFromMap becomes an order-tolerant
merge: new records are placed from the captured fields; existing lure
records only receive GMO-owned facts (verified despawn time).

Also guard loadPokestopFromDatabase against nil database to allow tests
with unknown forts to proceed without panicking.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Remove premature newRecord=false flip that violated the invariant:
newRecord transitions to false only in getOrCreatePokemonRecord
(rehydration) and savePokemonRecordAsAtTime (post-save). The early
flip in updateFromMap caused saves to see genuinely-new lure placements
as existing records, queuing spurious tree removes instead of inserts.

Update TestUpdateFromMapMergeAddsVerifiedExpiryOnce to simulate the
production commit between updateFromMap calls, matching existing test
patterns (TestUpdateFromMapLeavesNonLureRecordsAlone).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The request carries the encounter id, fort id and fort coordinates, so
a disk encounter no longer needs a prior GMO: it creates a fully placed
lure_encounter record with a 180s unverified expiry estimate (lure
spawns live 3 minutes), which a later GMO tightens to the verified
despawn. The request is now essential, matching GetStationDetails; the
DisplayId encounter-id stand-in is gone.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Apply the codebase convention for proto unmarshaling: use the
unmarshalClientProto helper instead of raw proto.Unmarshal. This
provides ~3.7% faster decode rate and 5.5% fewer allocations via
DiscardUnknown, consistent with every other request/response decode in
the file (e.g. decodeGetStationDetails, decodeTappable).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Disk encounter responses are processed directly against the record
created from their request proto; nothing waits for a GMO anymore.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… metrics, comments)

Five review fixes on the request-first lure pipeline:
- Eliminate the decoder-test statsCollector data race: set the noop
  collector once in init_test.go's init() instead of per-test in
  lureTestSetup, since the async stats aggregation worker reads the same
  global concurrently with per-test reassignment.
- Guard UpdatePokemonRecordWithDiskEncounterProto against a SUCCESS
  response with a nil PokemonDisplay, which otherwise panics downstream
  in addEncounterPokemon.
- Count DiskEncounter requests with encounter id 0 as a metrics error in
  decodeDiskEncounter, instead of the decoder-side early return going
  unnoticed after an "ok" increment; keep the decoder-side guard as
  defense in depth.
- Correct a false comment in decodeDiskEncounter about the HTTP path's
  base64 decoding of an empty request (decodeBase64Pooled("") returns
  nil, not a non-nil empty slice).
- Document why loadPokestopFromDatabase treats a nil GeneralDb as
  sql.ErrNoRows (zero-value test DbDetails{}; never hit in production).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Mygod

Mygod commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Thanks — this looks like it fixes #390 and substantially improves the request-first DiskEncounter path. However, I found a separate live-wire issue that this PR does not currently cover.

I captured and decoded current GET_MAP_OBJECTS traffic:

  • 17,009 GMO payloads
  • 67 unique lure Pokémon in fort.ActiveFortPokemon
  • zero occurrences of singular fort.ActivePokemon
  • 65 of those lure IDs later received a successful DiskEncounter
  • none appeared through Nearby, Wild, Catchable, Tappable, or normal Encounter paths

At aebc868, decodeGMO still extracts only singular fort.ActivePokemon:

Golbat/decode.go

Lines 553 to 561 in aebc868

if fort.ActivePokemon != nil {
newMapPokemon = append(newMapPokemon, decoder.RawMapPokemonData{
Cell: mapCell.S2CellId,
Data: fort.ActivePokemon,
Timestamp: mapCell.AsOfTimeMs,
FortId: fort.FortId,
Lat: fort.Latitude,
Lon: fort.Longitude,
})

Consequently, PR #391 should make Disk-encountered lure Pokémon visible using the request coordinates, but:

  • lure Pokémon without a DiskEncounter remain absent;
  • repeated-only GMOs cannot provide the verified ExpirationTimeMs;
  • the new GMO placement/merge logic is never reached for the current wire format.

There being only one active lure Pokémon per stop is compatible with this: the repeated field simply contains one LURE wrapper while the singular field remains nil.

I think the repeated-field ingestion should be tracked separately from #390. The eventual fix should retain singular support, iterate ActiveFortPokemon, unwrap non-nil PokemonProto entries, filter SpawnType_LURE, and deduplicate by encounter ID if both representations appear.

A decodeGMO regression test with ActivePokemon == nil and one repeated ActiveFortPokemon entry would catch this. The current tests construct RawMapPokemonData directly and therefore bypass the missing extraction step.

Live captures (17k GMOs, PR #391 review) show current clients deliver
lure pokemon exclusively in the repeated fort.ActiveFortPokemon wrapper
(SpawnType LURE, nested MapPokemonProto with zero lat/lon) — the
singular fort.ActivePokemon this code extracted is always nil, so the
GMO lure path never fired on live traffic and un-encountered lures were
invisible. Extraction now unwraps LURE entries from the repeated field,
keeps legacy singular support, dedupes by encounter ID, and filters
POWER_UP wrappers. Placement already comes from the enclosing fort's
coordinates, which is required: the nested proto's own lat/lon are zero
on the wire. Regression-tested against a sanitized live-capture fixture
(testdata/gmo-active-fort-pokemon.pb).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jfberry

jfberry commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed and fixed in 23a2334 — thanks for the capture work, and to the fixture for making this trivially testable.

Root cause matched your analysis exactly: extraction only read the singular fort.ActivePokemon, which is nil on current wire traffic; lure pokemon arrive as repeated fort.ActiveFortPokemon wrappers (SpawnType == LURE, nested MapPokemonProto). Notably the nested proto's lat/lon are zero on the wire, so the enclosing fort's coordinates are the only usable ones — the capture-at-extraction design this PR introduced already supplies them, so the fix stayed contained to the extraction layer.

What landed:

  • extractFortMapPokemon unwraps LURE entries from the repeated field, retains legacy singular support, dedupes by encounter ID when both representations appear, and filters POWER_UP wrappers (nil wrapper/nil inner proto safe).
  • Extraction-layer regression tests driven by a sanitized live-capture fixture (testdata/gmo-active-fort-pokemon.pb) with ActivePokemon == nil and one repeated LURE entry — addressing the gap you pointed out where the existing tests constructed RawMapPokemonData directly.

Folded into this PR rather than tracked separately since without it the PR's GMO merge path is dead code on current traffic.

@jfberry
jfberry merged commit d5e33dd into main Jul 29, 2026
7 checks passed
@jfberry
jfberry deleted the fix/lure-request-first branch July 29, 2026 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lure Pokemon becomes unqueryable after Pokestop lookup failure Disk (lure) encounter cache no longer needed

2 participants