perf: shrink cached Pokemon 800→352 bytes (field packing, plain-struct scan history)

[TurtIeSocks]

Shrinks the cached Pokemon struct from 800 bytes to 352, moving it from Go's 896-byte allocator size class to the 352-byte class — 544 fewer bytes per cached pokemon. PokemonLookup, the per-candidate scan struct, also drops 18 → 16 bytes. Measured on production instance under live traffic:

	main	this branch
pm2 RSS	15 GB	9.2–9.3 GB (−38%)
GC mark CPU	24.22%	~19%
live heap	6.66 GB	~4.2 GB
pokemon	896 B/obj	352 B/obj
spawnpoint	144 B/obj	112 B/obj

String interning was split out to #395 at @jfberry's request, so it is no longer part of this change.

Why 512 bytes

Go's GC treats objects above 512 bytes materially worse. Measured across 5M live entries with forced GC, mean of 5 runs:

object shape	GC mark
800 B, 12 pointers (the original Pokemon)	279.7 ms
800 B, 1 pointer	271.6 ms
800 B, 0 pointers	62.4 ms
512 B, 1 pointer	62.6 ms
520 B, 1 pointer	194.4 ms
256 B, 1 pointer	33.2 ms

Dropping pointer count from 12 to 1 buys 3%. Adding 8 bytes across the 512 boundary costs 3.1x. TestPokemonUnderGCThreshold guards the line.

Approach

Nullable fields use null.Value[T]. guregu/null's non-generic types are 16 bytes each because they embed sql.NullInt64, and most nullable pokemon columns are tinyint or smallint. null.Value[T] embeds stdlib sql.Null[T] and measures 2/4/8/16/8/2 bytes for uint8/uint16/uint32/uint64/float32/bool. Scan range-rejects and encoding/json renders float32 at 32-bit precision, so both behaviours an earlier draft hand-rolled come from the library.

cell_id and spawn_id are null.Value[int64]. sql.Null[uint64] cannot scan a negative int64 (cell_id is a signed bigint whose real values are frequently negative), and its Value() rejects any uint64 with the high bit set.

Scan history is a plain Go struct. Pokemon embedded grpc.PokemonInternal, a generated protobuf message used purely as in-memory Ditto state, carrying MessageState/SizeCache/UnknownFields at 64 bytes per pokemon and 88 per scan — and only marshalled behind four ANDed conditions with the controlling flag defaulting off. Now []*pokemonScan, 24-byte header and 44 bytes per entry, with protobuf built on demand at the two DB boundaries. Elements stay behind pointers deliberately: the Ditto code holds a pointer to a history entry across other work and mutates through it, so a value slice would have &s[i] invalidated by append.

Fields ordered by descending alignment, and three deleted: Capture1/2/3 (setters with no callers, in neither the select columns nor the upsert), changedFields (const-folded dead in production builds, but its 24-byte header and one GC-scanned word stayed), and seen_type as a string (an eight-value enum in a 24-byte null.String plus a heap pointer, now a one-byte code).

The debug change accumulator rolled out to every entity. Four crossed an allocator class — Station, Spawnpoint, Incident, Tappable. Pokestop, Gym, Route and Player shed 24 bytes but stayed in the same class, so they save nothing at allocation time.

What the profile shows

The first version of this description estimated the CPU saving at "low single-digit percent," sourced from docs/decode-performance-findings.md. That 5.4% figure is from the protobench harness, not production, and was wrong by roughly 5x. @jfberry's "absolutely will need to be benchmarked" was the right call.

Measured on main: GC mark 24.22% of CPU, live heap 6.66 GB against ~14 GB RSS — so about half the process was collector headroom, with GOGC unset. Pokemon were 2.71 GB of that, 40.8%, at ~3.25M cached.

On this branch, per-entity bytes match the predicted size-class ratios to three decimals:

pokemon      896.0 -> 352.0 B/obj   ratio 0.393   size-class predicts 0.393
spawnpoint   144.0 -> 112.0 B/obj   ratio 0.778   size-class predicts 0.778
scan entry    55.8 ->  25.4 B/obj   ratio 0.455   size-class predicts 0.500

Scan entries beat prediction because they also lost their protobuf machinery, not just a size class.

The RSS win outran the struct arithmetic by 1.6x. The model said 1.84 GB of live heap saved, doubled for GOGC=100 headroom, so ~3.7 GB of RSS. pm2 shows 5.75 GB. The difference is second-order and the model did not include it: fewer and more uniform objects mean less GC metadata and less span fragmentation, and HeapReleased went from 8 MB to 647 MB — the runtime is now returning pages where before it essentially was not.

Caveats on the measurement

The two captures were taken at different points in the day. main at 19:17 (evening peak, 3.25M pokemon), this branch at 07:40 (morning trough, 2.74M). That gap is diurnal, not cache warmth. Adjusting for 0.51M more pokemon at 352 B with the GOGC doubling puts the branch near 9.6 GB at peak — still ~5.4 GB and 36% below baseline, but a capture at ~19:00 would settle it properly.

The GC figure is two samples, 18.56% and 19.58%, against a baseline that was a single 30s sample. Read it as ~19% ± 0.5 and ~5 points saved, not 5.7.

The capture is ~25 commits behind. Every production number here was taken on this branch at bca1669. Four review rounds have landed since, all of them correctness fixes rather than size changes, so Pokemon is still 352 — but the figures have not been re-observed.

Nothing has been measured on go 1.27, whose allocator changes could move all of this. Raised by @jfberry and still open.

Wire changes

API responses: weight, height and iv render with fewer digits — 6.7 rather than 6.699999809265137. They originate as protobuf float fields promoted through float64(), so the extra digits were an artifact of that promotion. Verified byte-identical across 500,018 values through both the stdlib and goccy encoders.

Webhook payloads: same fields, same change.

The OpenAPI schema changed on 19 fields, and separately on four timestamps — 16 integers format: int64 → int32, 3 floats double → float. Worth calling out separately: narrowing to unsigned Go types also made huma emit minimum: 0 on those 16 fields. That is a validation constraint rather than a format label, so a strict client or validating proxy would begin rejecting negatives. Accurate to the domain — none of those fields can legitimately be negative — but it is new contract surface beyond the format change, and it was not part of the original ask. TestApiPokemonResultSchemaWidths now pins the shape so a future width change cannot drift it silently.

A later round then took the four timestamp fields the other way. expire_timestamp and updated had become format: int32, which overflows generated clients in 2038 — and inconsistently, since first_seen_timestamp stayed int64. All four are now format: int64 with minimum: 0. Note the second half of that: first_seen_timestamp and changed gain a floor they never advertised, because huma had nothing to infer one from at int64. Accurate to the domain, but new constraint surface.

seen_type enum strings and the golbat_internal protobuf are unchanged.

Review feedback

Round 1 — null already supports generics (the decoder/nulltypes package an earlier draft added is deleted, −702 lines); pull the SeenTypeCodeWild = 0 fix in (code 0 is now an explicit Unset sentinel); the webhook re-widening is wasted (gone); the embedded protobuf is ripe for change (done, stacked here); rename the change list and roll it out (done).

Round 2 — four asks, all addressed:

1. SetSeenType takes a SeenTypeCode, not a string. The constants are typed, so a typo now fails to compile instead of silently no-opping, and the decode path loses a map lookup.
2. The widen layer is gone. widenPtr/widenFloatPtr deleted across 19 call sites by narrowing the response types.
3. Clamp comparisons converge. This was a real bug and the most serious thing in the round — see below.
4. NullSeenType.Scan degrades instead of failing. An unrecognised enum value now warns and yields invalid rather than erroring, because an erroring Scan stranded a newRecord cache entry that re-failed its DB load on every sighting, silently dropping all processing for that pokemon for the duration of a mixed deployment. Value() stays strict — writing an out-of-enum string to a MariaDB ENUM is silently stored as ''.

On #3, since it was introduced by this PR

Several sites compared a clamp-saturated stored value against a raw proto value. With a costume of 300, the stored value saturates at 255 and 255 != 300 on every sighting, forever. The branch that guards nulls out weight, height, size, moves, cp, shiny, ditto and pvp — so every encounter's enrichment was undone by the next sighting, fleet-wide.

An earlier review on this branch flagged it as Minor and "not reachable with today's proto values," and it shipped on that basis. That was wrong twice over: pogo enums are open int32, so unknown and negative wire values pass straight through, and Golbat ingests raw protos from third-party scanners — a malformed packet is enough.

Fixed at 7 sites across 5 functions (3 of which the ask did not name) with non-counting narrowUint8/16/32 helpers on comparison sides, the counting clampUint* delegating to them so golbat_field_clamped_total still fires exactly once per real store. Regression tests cover each site individually — reverting one fails only its own test.

Round 4 — four asks plus a ranked list, all addressed. Two of the four turned out deeper than stated:

• An unrecognised seen_type was degrading to Unset, which is an active state. updateFromWild downgraded the record, updateFromNearby replaced precise coordinates with cell centres, and the save wrote the damage back over a newer binary's value. Pre-PR main stored the string opaquely and every switch fell through harmlessly, so the degrade shipped worse than what it replaced — in exactly the mixed-deployment case it was built for. Now a distinct unknown code, inert at all 17 switch sites, with COALESCE on both write paths so it refuses to overwrite.
• calculateIv computed Iv from the raw IV sum while storing clamped values. iv is float(5,2) unsigned, so a sum above 450 produces a value MariaDB rejects under STRICT_TRANS_TABLES — failing the entire multi-row batch upsert. Narrowing the inputs as specified did not close it, because the clamp used the column's 255 rather than the game's 15. Clamping at 15 does, and subsumes it: sum ≤ 45 means Iv ≤ 100.

Chasing that second one surfaced a third bug nobody had connected: PokemonLookup's int8 fields use -1 for "absent", and int8(255) is -1. A clamped value entered the scan index as the sentinel for never encountered, silently changing which DNF filters matched. Every lookup field now saturates at a sentinel-safe ceiling instead of truncating — covering the whole ≥128 band, not just the exact ceiling — and Form gets its own helper because its -1 is the wildcard-form key rather than an absence marker. Atk/Def/Sta/Iv were reachable from rows already in the database, since a write-side clamp cannot heal stored data.

CI also now runs under the production build tag. It was running go test ./... without -tags go_json, so every golden-JSON proof in this PR was verified against stdlib while production serves through goccy. Nothing failed once corrected — goccy matches byte-for-byte across every fixture and adversarial case tested — but the proofs were unenforced against the shipping codec. A MariaDB service container now makes the round-trip tests actually execute rather than skip.

Rounds 5 and 6 — @Mygod flagged that the protobuf-to-plain-struct conversion drops unknown fields when an older binary rewrites golbat_internal. A guard was built to refuse the overwrite; @jfberry ruled it an overreach — the field is transitory, the flag defaults off, and losing fidelity there is harmless — so it was reverted. Four smaller asks landed instead: the proactive-IV skip now compares weather in one encoding rather than testing the lookup's -1 against the entity's 0; NullSeenType.UnmarshalJSON yields the inert Unknown sentinel rather than the destructive Unset; the warn throttles are reset by tests instead of swapped, closing a latent race; and three stale doc claims are corrected. CI also now vets the untagged build, which nothing else covered.

Testing

• Live MariaDB round-trip through the production pokemonBatchUpsertQuery: a row with every nullable column NULL, and a fully-populated row covering the full 64-bit cell_id, spawn_id, and float64 lat/lon precision.
• Wire compatibility for golbat_internal: a hand-written byte literal in the old grpc.PokemonInternal shape decodes through the new path, and the new write path produces bytes byte-identical to it.
• TestPokemonScanCoversEveryProtoField reflects over both structs and diffs exported field-name sets, so adding a proto field without mapping it fails loudly.
• Clamp convergence and clamp counting, each pinned separately.
• TestPokemonEntitySizes pins the sizes; TestPokemonUnderGCThreshold guards the 512 line.
• Green under both build tags, -race, golangci-lint, and the full suite.

statsCollector became an atomic.Pointer seeded with a noop at package init. A test needed to swap it, and decoder/init_test.go already documented that doing so races the stats aggregation worker, which reads the global on a ticker for the life of the process. This also retires a pre-existing race in station_battle_test.go.

Follow-ups

ProactiveIVSwitch's boosted-weather guard never fires — pre-existing on main, confirmed by @Mygod. boostedWeathers&uint8(1)<<w != 0 parses as (boostedWeathers & 1) << w, since Go gives & and << equal precedence. Every boostedWeatherLookup entry is even, so the guard is unconditionally false and newWeather is always 0 — the switch can only remove boosts, never apply them, and a currently-boosted pokemon whose new weather still boosts it gets wrongly de-boosted. Separately, NewWeather arrives as an unvalidated int32 from an open proto3 enum, and a negative shift count panics in a goroutine with no recover in its chain, so one malformed packet takes the process down. Both belong in their own PR, since the fix changes live behaviour immediately.

• The same negative-aliasing pattern is still live in fortRtree.go — bare int8/int16 casts on quest reward amounts, team and slots. The lookupInt8/lookupInt16 helpers added here apply verbatim.

• clampUint's derived ceiling would silently wrap for a future ~uint64 instantiation; a limit < 0 panic guard is cheap.

• The statsCollectorSet ordering guard doesn't cover the exported InitTypedQueues.

• TestUpdatePokemonLookupSaturatesClampedFields leaks a phantom entry into pokemonFormCount.

• The publish workflow's build-gate comment overclaims (branch pushes still publish per-branch images; the load-bearing case is fork-PR tokens), and tag-push releases now block on MariaDB service health.

• Bit-packing the numeric fields. Deliberately not done here. From 312 bytes, packing the small numerics into shared words reaches roughly 241 (the 256 class); adding a database row shim so Pvp and GolbatInternal can leave the struct reaches roughly 193 (the 208 class) — another ~112 bytes per pokemon. It has to be a build tag rather than a config flag, because Go fixes struct offsets at compile time and a runtime branch keeps the struct at the larger size, saving nothing. The cost is a build matrix of 4+ combinations and converting ~250 direct field reads to accessors in both layouts, so the readable build stops being readable. Worth its own PR against this measured baseline.

• Pvp and GolbatInternal were assessed and left alone. Two measured negative results, both now pinned in entity_sizes_test.go: Pvp is not dead between its write and its zeroing (the queue snapshot, the direct-write fallback and the webhook builder all read it), and narrowing it alone buys exactly zero allocated bytes, because 296 still lands in the 320 class and the next class down is 288.

• The intern table wants an alert threshold, not just a dashboard line. The length cap bounds entry size, not table count; a caller sending a million distinct well-formed usernames still grows it. golbat_intern_table_size is the instrument.

• A clamped value now lands exactly on PokemonLookup's missing-value sentinel — int8(255) = -1, and int16(65535) = -1 for Cp and Form. That aliases "clamped" with "unknown" in scan filters, which the pre-narrowing int8(300) = 44 did not. Narrow, latent, introduced here.

• Off-heap/arena storage for PokemonData deserves another look. It was ruled out earlier using ~5% for GC's share of CPU; at a measured 24.22% that arithmetic does not hold.

• Pokestop (1152 B) and Gym (968 B) are both far over the 512-byte threshold.

Design: docs/superpowers/specs/2026-08-16-pokemon-struct-packing-design.md

🤖 Generated with Claude Code

[jfberry]

Absolutely will need to be benchmarked; and against go 1.27 which changes allocation to be more efficient anyway.
But the memory saving alone may make this worthwhile

[jfberry]

Incidentally, since the null package already supports generics you may not need the newly defined nulltypes but rather just right-size in the current package

The embedded protobuf is absolutely ripe for change, it's a smell that it's in there in protobuf form - I did once replace it with a conventional structure and move to protobuf on demand (almost nowhere --> it is only marshalled when writing to disk, and the default is that it isn't).

The pokemonChanged value could be renamed to a generic name to encapsulate the change list, and rolled out to the other objects for a near free improvement.

[jfberry]

Finally, pull the SeenTypeCodeWild = 0 fix into this PR - and the re-widening of the webhook to float64 and reconversion seem like they are wasted steps for some misattributed compatibility requirement

[jfberry]

Thanks for the update — the null.Value[T] swap, the seen-type sentinel, and the accumulator rollout all landed cleanly, and the round-trip/size tests held up under a full re-review (both build tags, -race). A few asks before undraft:

1. SeenType: pass the enum, not the string. SetSeenType(s string) runtime-parses the SeenType_* constants through the string→code map at all 12 call sites, and a typo'd string compiles and silently no-ops. Now that SeenTypeCode owns the representation, make the constants typed and have SetSeenType(SeenTypeCode) — the string should only be produced at output boundaries (DB Value(), webhook build, API marshal). Removes a map lookup from the decode hot path and settles the current split between string comparisons and .Code comparisons.

2. Delete the widen layer in API results. widenPtr/widenFloatPtr (19 call sites) exist only to keep ApiPokemonResult at *int64/*float64. Narrow the response fields to storage widths instead: encoding/json already marshals float32 at 32-bit shortest form — exactly what widenFloatPtr's FormatFloat+ParseFloat round-trip reimplements at ~50 ns/field — so output is byte-identical and the layer is ~3× the cost of direct assignment on the highest-volume API path in the codebase.

3. Clamp comparisons must converge. setPokemonDisplay and wild/nearby SignificantUpdate compare the clamp-saturated stored value against the raw proto value. The day a costume exceeds 255 or a form 65535, every sighting becomes a "significant update" — and the changed-branch wipes encounter data after every enrichment, fleet-wide, until a code change. Compare through the same narrowing (a non-counting clamp) so comparisons settle the way raw int64 storage did.

4. SeenType Scan should degrade, not fail. The enum has been widened three times (migrations 3, 43, 45). If a newer binary writes a 9th value and this one reads it (rollback, lagging replica), the Scan error strands a newRecord cache entry that re-fails its DB load on every sighting and drops all processing for that pokemon. Map unknown → invalid + warn, matching the proto path's existing precedent.

Explicitly fine: the pokemon.Weather = ... → SetWeather(...) change in ProactiveIVSwitch alters behavior (the old memory-only assignment left PokemonLookup stale), but we think the new behavior is correct — keep it, just add a comment marking it intentional.

Smaller, take or leave: collapse clampUint8/16/32 into one generic; narrow webhook Shiny and drop nullBoolToGuregu; an int64OrZero[T] helper for the 45 int64(x.ValueOrZero()) casts (hoisting the cast outside a subtraction compiles and wraps); seed the statsCollector atomic with the noop collector so the nil contract disappears; Player's save is missing debug.reset(); SetSeenType's dbdebug line should use FormatNull like its siblings; the two nullscan tests duplicate the DB scaffold; and entity_sizes_test.go references review-task-4-report.md, which doesn't exist in the repo.

[jfberry]

Thanks — all six asks from the last round landed exactly as specified, and the implementation quality held up under re-review (both build tags, -race, full suite green). Comments on the new interning work first, then two refinements to our own earlier asks, then the rest.

On the interning commits

The implementation is genuinely good — the publication-order argument is correct, reject-not-truncate is the right policy with the right rationale, and the Value() degrade reasoning is exactly right (you independently found the same batch-poisoning mechanism we flag below on calculateIv). But we'd like to redirect this rather than grow it here:

Split the interning into its own PR. Not because it's unwanted — the opposite: it has more general applicability than this PR can give it. Fort-id interning is already pre-scoped as the scaling lever for the fort R-tree (dense integer keys for both the tree and the lookup map), and a shared intern subsystem should be designed once with that use in mind, not embedded ad hoc where the fort use case can't shape it. Splitting also lets this PR — which is three rounds deep and converging — undraft on the strength of what's already reviewed. A word of caution alongside: the wins are now an order of magnitude smaller per unit of complexity than round one (312 B in the 320 class bought ~100 MB where the first pass bought ~2.4 GB), and this round crossed the line from removing weight to adding a runtime subsystem. That's the point at which each further byte needs to justify a design, so let's not let the PR become a size-golf exercise — the packing result is already excellent.

Username may not need interning at all — question whether to record it. Its only load-bearing consumer is the shiny/duplicate-encounter stats dedup (updateEncounterStats), which needs an account-distinguishing token transiently at encounter time — and the decode context already has the username in scope. The persisted field is only ever set once (first account to see the pokemon) and otherwise just flows to the DB column and API response. If persisting it were config-gated (off by default or on, operator's choice), the harder half of the intern problem — a caller-supplied, optionally-unauthenticated, unbounded key set retained forever — disappears entirely rather than being engineered around. Pokestop_id is the strong half of the idea: genuinely finite, stable identifiers, already RAM-resident under fort_in_memory — keep that.

If username interning does stay: cap the count by construction. The file's own principle ("bounded by construction rather than by hope") is applied to string length but not to count, and username arrives off the request body with the RawBearer gate optional. Under the old representation a junk username was transient — evicted with the entity in about an hour; interned, it's resident until restart, at request rate. Past a table-size ceiling, degrade to the null handle + counter, exactly like every other failure path in the file. ~10 lines.

Refinements to our round-3 asks

Two of the things we asked for created new problems as specified — these are corrections to the asks, not misses on your part:

1. The Scan degrade needs a distinct "unknown" sentinel, not Unset. Degrading an unknown enum string to Unset makes it an active state: updateFromWild downgrades the record to wild, updateFromNearby's Unset/Cell case replaces precise coordinates with stop/cell centers, and the save unconditionally persists the downgraded seen_type over the newer binary's value. Pre-PR main stored the unknown string opaquely and every switch fell through to default, so the record round-tripped unharmed — the degrade is now more destructive than either prior behavior in exactly the mixed-deployment scenario it was built for. Add an unknown code that matches no rewrite cases and round-trips (or refuses to overwrite) at the write boundary. Also throttle the Scan warn — it fires per scanned row (millions during preload, under entity locks at runtime); util.DropReporter is the convention.

2. calculateIv must compute from the narrowed values. It stores clamped a/d/s but computes Iv from the raw sum, and the convergence fix makes the inconsistency permanent (the narrowed guard reports "unchanged" forever). The sharp edge: iv is float(5,2) unsigned, so a raw sum > 450 yields Iv > 999.99, which under MariaDB's default STRICT_TRANS_TABLES fails the entire multi-row batch upsert — the same blast radius your intern Value() comment describes, reached through data instead of a fabricated handle. Narrow a/d/s once at the top; use the narrowed values for the comparison, the stores, and the Iv computation.

Enforcement gaps

3. CI needs a go test job. This is now a hard ask: the PR's whole enforcement story is test-pinned invariants (the Scan asymmetry, the clamp-counting trap, the size classes, the schema widths), but CI runs only golangci-lint — a later commit reverting any of them merges green. Ideally add a MariaDB service container so the round-trip tests (currently skipped without GOLBAT_TEST_DSN) actually execute; they're the driver-level proof for the narrowed columns.

4. Golden JSON tests must run under the production encoder. They import stdlib encoding/json, but production marshals through goccy (huma_api.go + -tags go_json), so the byte-identical-narrowing proof never exercises the encoder that actually serves responses. Route the test marshal through an indirection that respects the build tag so both encoders are covered.

Ranked smaller items

These only trigger on out-of-range or unknown-enum inputs, but that's this PR's own stated threat model:

• ExpireTimestamp/Updated as *uint32 now advertise int32 in the OpenAPI schema (inconsistently — first_seen_timestamp stays int64); generated clients overflow at 2^31. Keep timestamps *int64 at the response boundary or add explicit format/maximum overrides.
• Clamped saturation values alias PokemonLookup's -1 "absent" sentinel through the mirror's truncating casts (int8(255) = −1), so a clamped level/weather/CP silently drops the pokemon from filtered scans. Saturate at the mirror level too.
• ProactiveIVSwitch's fast-path skip still truncates via int8 cast while the slow path narrows — two equivalence relations on the same value in adjacent lines. Narrow newWeather once at the top.
• SetSeenType lost its validation and the two output boundaries disagree on invalid codes: Value() errors (failing a whole write batch at bind time, no retry) while MarshalJSON silently emits "". A one-line guard in the setter closes both.
• The noop-seeding still has gaps: the init comment overclaims (package var initializers run before init()), SetStatsCollector(nil) is now a delayed panic, the db package's sibling collector is still nil-unguarded, and InitWriteBehindQueue's by-value snapshot makes mis-ordering silently noop where it used to fail loud.
• player.go: the new debug.reset() sits after the INSERT error early-return, so a failed insert re-logs stale changes on the next save. Consider folding reset() into ClearDirty() and deleting the ten copies (the move above the write is needed either way).
• TestSignificantUpdateConvergesOnOutOfRangeDisplay codifies a pre-existing wrong-field read: nearbySignificantUpdate compares PokemonId against DisplayId, which is an instance id, not a pokedex number (updateFromNearby itself uses PokedexNumber). Fine to leave the gate alone to contain scope, but the test comment shouldn't bless it as intended.
• TestNullSeenTypeScanUnknownValueDegrades swaps the global logrus writer and reads the buffer outside logrus's mutex while package-init goroutines can log concurrently — a -race flake waiting to happen. Use hooks/test.NewNullLogger or a dedicated logger.
• isSeenFromTappable returns true when the pokemon was not seen from a tappable; a future reader "fixing" the call site would destroy tappable attribution. Rename it.
• SetChanged is the one narrowed setter with a bare truncating cast (no saturation, no clamp metric), and its comment cites setters that store into int64-wide fields and can't truncate. Route through clampUint32 like SetUpdated.

Tidy-up batch, no discussion needed: dead narrowUint32; clampUint's limit derivable from T; the garbled comment at stats.go:267; webhooks.md still documents shiny as null.Bool; init_test.go's collector seed now redundant.

[jfberry]

One addendum on the interning follow-up, for whoever picks it up: pokestop ids are already integers. The string form is 32 hex characters + . + a short type suffix (the Ingress-heritage portal GUID format — which is exactly why the column is varchar(35)), i.e. a hex rendering of a 128-bit identifier plus a small discriminant. So the stronger model than a shared intern table may be a fixed-width value type — [16]byte + suffix byte — converted to the string form at the DB/JSON boundaries, precisely the way Uint64Str already handles pokemon encounter ids. No heap pointer, no global table, no shared mutable state, trivially comparable and hashable.

We've been down this road before and parked the incident-id version of it as #384, which also documents the one real risk that applies here too: parse-on-ingest must be fallible with a graceful fallback (a nonconforming id from another data source or a Niantic format change must degrade, not disappear).

To be fair about the trade on this struct: the byte array costs ~13 more bytes per pokemon than the 4-byte handle and would push 312 back into the 352 class — so for the cached Pokemon alone the handle is tighter. The byte-array model's real payoff is the fort subsystem, where there's no table at all: fort cache keys, FortLookup map keys, and the R-tree entries (the pre-scoped fort-scan scaling lever) all become fixed-size value keys with no hashing of 35-byte strings and no GC-visible key pointers. Worth evaluating both shapes — or a combination — in the follow-up PR rather than committing to the intern table as the foundation.

[Mygod]

The protobuf-to-plain-struct conversion introduces forward-compatibility data loss when internal data is rewritten by an older binary. Otherwise, the targeted production and dbdebug decoder tests passed.

Review comment:

• [P2] Retain protobuf unknown fields during scan conversion — decoder/pokemon_scan.go:152-159
  When pokemon_internal_to_db is enabled during a rolling upgrade or rollback, rows may contain fields unknown to this binary. proto.Unmarshal previously retained those bytes in pokemon.internal, but rebuilding fresh PokemonInternal and PokemonScan messages here copies only known fields, so the next encounter overwrites golbat_internal without the newer data. Preserve the unknown bytes or message state when rebuilding.

Arguably doesn't need fixing?

[jfberry]

This (protobuf defence) change is an overreach. This is a transitory field, and there is no harm in losing fidelity when the default isn't even to store it.

[jfberry]

Round-6 verification is done and it's clean: both blockers are genuinely fixed and were verified empirically — the CI job is green with the MariaDB round-trip tests actually executing (not skipped) under both production tag sets, calculateIv now computes comparison, stores, and Iv from the same clamped values for every input, and the Unknown sentinel was traced through every non-test reader of SeenType and is inert at all of them, with COALESCE refusing overwrites at both write boundaries. All ten ranked items, the tidy-ups, and the title/body correction landed. Nothing in the new commits is merge-blocking — the undraft stands from our side. A few small items below, plus one thing that needs an author who predates this PR.

@Mygod — your advice wanted on ProactiveIVSwitch (pre-existing, not this PR's doing)

The review's differential pass sat on top of your boosted-weather guard and found something we'd like your read on before anyone fixes it, since you wrote this and know the intended behavior:

if boostedWeathers&uint8(1)<<weatherUpdate.NewWeather != 0 {
    newWeather = weatherUpdate.NewWeather
}

Go gives & and << equal precedence (left-assoc), so this parses as (boostedWeathers & 1) << NewWeather, not boostedWeathers & (1 << NewWeather). Every boostedWeatherLookup entry is even (bit 0 is weather NONE), so boostedWeathers & 1 is always 0 and the guard is unconditionally false — confirmed empirically, and it's on main, not introduced here. Net effect: newWeather is always 0, so the switch can only ever remove boosts, never apply them — and a currently-boosted pokemon whose new weather still boosts it gets wrongly de-boosted via repopulateIv(0, ...). The one-character fix (boostedWeathers&(uint8(1)<<weatherUpdate.NewWeather) != 0) changes live behavior immediately, which is why we'd rather have your confirmation of intent than just land it.

While you're looking: NewWeather arrives as an unvalidated int32 from an open proto3 enum, and a negative value used as a shift count panics — in a goroutine with no recover in its chain, so one malformed packet from any third-party scanner takes the whole process down. Clamping/validating to 0..7 before the shift (or making it uint8 at construction) closes both the crash and half the precedence question at once. Happy to take both in a follow-up PR if you agree with the intended semantics.

Small asks for this PR

1. The weather fast-path still compares two encodings of "absent". The cheap skip tests narrowUint8(newWeather) == pokemonLookup.Weather, where the lookup encodes NULL weather as -1; the entity-level guard after the lock uses int64OrZero(pokemon.Weather), which encodes NULL as 0. So for a pokemon with encounter values but NULL weather, a no-boost update (newWeather == 0) compares 0 == -1, fails the skip, takes the entity lock — and then the guard's 0 != 0 does nothing. One wasted entity lock per NULL-weather pokemon per weather flip in its cell, which is exactly the lookup-vs-entity disagreement the new comment says was closed. Fix: do the skip comparison in the lookup's own encoding (map "no weather" to -1 on the incoming side, or normalize the lookup value to 0 before comparing).

2. NullSeenType.UnmarshalJSON disagrees with Scan about the degraded state. Scan on an unknown string produces Unknown (inert everywhere — good), but UnmarshalJSON(null) produces code 0 = Unset, which is precisely the code this PR proved destructive (it licenses updateFromWild's rewrite and updateFromNearby's coordinate replacement). No production path unmarshals it today, but the type is exported and is the webhook payload's field type, so any replay tool or future path that round-trips a degraded record converts the safe sentinel into the destructive one. Make the two boundaries agree deliberately — one line. (The read-side behavior itself — degraded rows serving seen_type: null during a mixed-deployment window — is fine as-is; the column is COALESCE-protected.)

3. The warn-throttle globals — fix on the test side if possible. seenTypeScanWarns / seenTypeSetWarns are plain package-level pointers that production reads on row-load/decode paths while tests reassign them unsynchronised — the same shape init_test.go's own comment documents as a former data race for statsCollector. It's quiet today only because nothing loads rows concurrently with the swap. Preference: fix it without touching production code — e.g. have the tests inject their own DropReporter through a test-only seam rather than reassigning the global. If there's no clean way to do that, atomic.Pointer[util.DropReporter] is acceptable, but the production code shouldn't grow complexity to serve a test if it can be avoided.

4. Docs. Scan's comment overclaims the self-heal: wild and lure sightings deliberately don't match Unknown (correctly — matching it would reintroduce the downgrade), so the record heals on encounter or on reload only; reword it, because a reader making the documented behavior "real" by adding case SeenTypeCodeUnknown: to updateFromWild would rebuild exactly the bug the sentinel prevents. Also: webhooks.md's type table still lists seen_type as null.String (the one stale row in a table this PR otherwise corrected), and TestNullSeenTypeJSON's comment names huma as the consumer when the webhook sender's stdlib encoding/json is the real one.

Reviewed and accepted

The IV clamp-at-15 behavior (a hypothetical out-of-range reading becoming a stored 15) was considered and is fine: we don't receive fake IV data in practice, and golbat_field_clamped_total is sufficient signal if that ever changes. No action wanted.

Noted, non-blocking

CI no longer builds the untagged (!go_json) configuration anywhere — the one a bare go test ./... uses, including half of jsonenc; one extra untagged step (even go vet ./...) covers it. The same negative-aliasing pattern just fixed in the pokemon lookup is still live in fortRtree.go (bare int8/int16 casts on quest reward amounts / team / slots — the lookupInt8/16 fix applies verbatim). TestUpdatePokemonLookupSaturatesClampedFields leaks a phantom entry into pokemonFormCount (pair the cleanup with adjustPokemonFormCount(-1)). The statsCollectorSet guard doesn't cover exported InitTypedQueues (queues reading the collector at use time would delete the flag and the ordering requirement together). The publish workflow's build-gate comment overclaims (branch pushes still publish per-branch images; the load-bearing case is fork-PR tokens), and tag-push releases now block on MariaDB service health. clampUint's derived ceiling would silently wrap for a future ~uint64 instantiation (a limit < 0 panic guard is cheap). And the two throttle tests assert an exact count of 1 against a real 1-second wall-clock window — a rare-flake on loaded runners; assert >= 1 and < rows or inject the clock.

[Mygod]

Damn why is a bot tagging me and roasting my code. Yes, that should be fixed.

[jfberry]

On golbat_internal, settling this fully — including f0d4560a, not just the earlier defence: we don't need backward compatibility here in either direction. This is transitory operational state, off by default, and the contract we want is the simple one: load it and use it if it parses; if it doesn't, blank is fine; and when we write, we write this binary's view of the world unconditionally. The state evolves with the binary — last-writer-wins is the intended semantic, not an accident to defend against.

So please drop the refuse-to-overwrite guard as well (storedInternalHasUnknownFields and its skip path): 62dd77c5 was the right direction, and f0d4560a re-adds a defence we're deliberately choosing not to take. Beyond the ~100 lines, it costs an extra proto.Unmarshal of the stored bytes on every gated encounter save, and during the very window it protects, it prevents the running binary from persisting its own scan history on rows a newer build touched — which matters more to the running binary than preserving bytes it can't read. Plain rewrite, no guard.

[jfberry]

Correction to my previous comment: on verification, 62dd77c5 had already reverted f0d4560a completely — no refuse-to-overwrite guard exists at head (storedInternalHasUnknownFields, the reporter, the metric, and its tests are all gone, zero leftovers), and the net effect of the pair is just the rewriteGolbatInternal extraction, which verified behavior-identical to the previous inline block. So golbat_internal is already exactly where we want it — treat my previous comment as the policy record, not an ask. Apologies for the churn.

Verification of the rest of the post-round-6 commits — all four asks landed correctly: the weather skip and entity guard now agree on "unchanged" for every input (NULL-weather case included); UnmarshalJSON(null) produces the inert sentinel, re-traced through every decode switch and pinned by test; the throttle race is genuinely eliminated; and all three doc corrections check out against the code. The untagged go vet step runs and passes, and the range assertions landed.

One deviation to accept explicitly: the throttle fix value-types the two globals and adds a small Reset() to util.DropReporter — technically production surface, where the ask was test-side-only. But it removes the pointer indirection that existed solely for the test swap, so it meets the actual constraint (don't compromise main code — this simplifies it) and is safer than the atomic.Pointer fallback we'd sanctioned. Fine as landed.

Residual nits, take or leave, none blocking: the SetSeenType throttle test's upper bound (< 4) has much less headroom than its sibling's (< 500) against a badly-preempted runner straddling wall-clock seconds; TestNullSeenTypeUnmarshalNullIsInert marshals via stdlib encoding/json where the file's convention is jsonenc.Marshal; webhooks.md's seen_type prose says null is emitted "when unset" and should add the degraded-value-during-mixed-deployment case; and DropReporter.Reset's two stores aren't atomic as a unit — worth a doc line that callers must quiesce reporters first (only tests call it today).

That closes the review from our side.

[Mygod]

No actionable regressions were found in the changed decoding, persistence, API, or metrics paths.