feat: add geocoder build pipeline with incremental patching - #235
Draft
zackpollard wants to merge 560 commits into
Draft
feat: add geocoder build pipeline with incremental patching#235zackpollard wants to merge 560 commits into
zackpollard wants to merge 560 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![don-t-switch-my-sha[bot]](https://avatars.githubusercontent.com/u/11602424?s=60&v=4){kind=small}
don-t-switch-my-sha
Bot
dismissed
their stale review
March 31, 2026 15:35
All action SHAs now verified.
zackpollard
added
the
planet-test
query_geo early-returned only on missing geo_cells, then unwrapped street_entries/ways/nodes — panicking on a partial/admin-only index. Guard them with a let-else that returns empty, matching the addr/interp paths. Behaviour-neutral for well-formed indexes (street files always accompany geo_cells), so it only converts a would-be panic into graceful degradation. Validated: grid-vs-HEAD identical over 327,593 planet coords; 50/50 tests.
Three verbatim copies of the close-ring + rotate-to-min-vertex canonicalization (s2_helpers admin-cover, build_index admin assembly, build_index POI rings) collapse into geometry.h canonicalize_ring_rotation(). Logic moved character-for-character.
The four PrimitiveBlock decoders each inlined the same string-table extraction loop; factor into decode_string_table(). Verbatim move.
builder-ui/static/configurations.json is a generated per-build manifest (template + per-file sha256/sizes spliced from manifest.json), regenerated and uploaded to Tigris every CI build. Production fetches the live one from the Tigris origin; the checked-in copy was only a local-dev convenience and produced churny hash-only diffs. The source of truth is the tracked template geocoder/builder/configurations.template.json. Untrack it and gitignore. Local dev now needs a configurations.json provided (fetched from Tigris or produced by a build) rather than the stale snapshot.
A cell's entry list is count-prefixed with a uint16 on disk; more than 65535 entries silently wrapped the count (the old cap used MAX_VERTEX_COUNT = 0x7FFFFFFF, a no-op for a 16-bit field) and the server would under-read the cell. Replace the warning with a hard build failure via checked_entry_count(), checked before any bytes are written (the threaded writer records per-chunk maxima and throws after join). Also log "max entries/cell" per entries file every build as an early-warning canary. Planet headroom today: max 29,975 (poi/all, 46% of the limit), everything else <=10k. Widening to u32 later is a routine build_version bump (one no-patch day), so deferring the format change until the canary shows real approach is safe. Byte-identical on oceania (122/122 vs validated HEAD output); geocoder-tests 99/99.
- link_poi_smallest_admin(data, num_threads, level_ok, init, store): the smallest-containing-admin-polygon PIP sweep shared by compute_poi_parent_postcode (level == 11) and compute_poi_parent_admin (levels 2..10), moved verbatim including the inline PIP and area tie-break. - for_each_named_street_segment(data, plat, plng, scratch, visit): the 5x5 cell-ring expansion + point-to-segment distance sweep shared by backfill_addr_point_parent_streets and compute_poi_parent_streets; callers keep their own candidate selection including the deterministic osm_id tie-break (and the backfill's token-match layer). -107 lines. Byte-identical on oceania (122/122 vs validated HEAD output); geocoder-tests 99/99. Planet parity covered by the batch validation.
Index::load treated every non-street file as individually optional, so a corrupt or partially-synced index dir (e.g. poi_records.bin present but poi_cells.bin missing) loaded "successfully" with the feature silently disabled. Add mmap_group(): files that are only meaningful together load all-or-nothing — addr[entries,points], interp[entries,ways,nodes], poi[cells,entries,records,vertices], place[nodes,cells,entries], postal[polygons,vertices], and the postcode-centroid triple. A whole group absent stays valid (admin/admin-minimal/no-addresses variants); half-present is now a hard load error naming the missing files. addr_vertices (v9+) and the parent/postcode single sidecars remain individually optional for old-index compatibility. Validated: negative-load tests (full symlink dir loads; poi_cells removed -> clean "Partial index group" rejection; whole poi group removed -> loads); 327K-coord grid identical vs baseline; 50/50 tests.
The road-selection cascade decides a POI wins primary using the tier-1 landmark boost (compare_poi_dist), but poi_won_primary — which routes resolve_postcode to the POI's parent_postcode vs the street's — was recomputed with the un-boosted distance. A tier-1 polygon POI that won primary only via the 50m boost surfaced the POI's road but the street's postcode. Set the flag inside the same cascade branch that picks the road, so the two decisions agree by construction (Nominatim routes the whole address, postcode included, through whichever feature wins). Validated: 327K-coord grid identical vs baseline (the disagreement window is empirically empty on the real grid — this closes it structurally); 50/50 tests.
Nominatim returns rural-US ZIPs from the TIGER segment matched to the winning street (location_property_tiger.postcode); we only kept TIGER ZIPs as aggregate centroids, losing the per-segment association — the root cause of the ~52% rural-US "no postcode" divergence. Builder: keep each TIGER row's ZIP in interp_postcode_ids (parallel to interp_ways, NO_DATA for OSM interpolations), carried through the deterministic-ordering dedup, strategy-2 reorder (strict size gate) and continent filtering; written as interp_postcodes.bin (u32 string offset per interp way). The sidecar exists iff it carries at least one real ZIP — non-TIGER builds/continents ship no file. The centroid pipeline is untouched. Patch tools: new sparse-delta section INTERP_POSTCODES (string remap, like way_postcodes); GCPATCH_VERSION 2->3 so pre-v3 appliers reject the whole patch upfront instead of failing mid-apply on an unknown section (v2 patches remain readable via GCPATCH_MIN_READ_VERSION); introduction day (no old file) emits full-replace instead of a 2x-size sparse delta. Server: postcode tier 1c — when the street wins the primary and the nearest interpolation segment lies within ~100m with a normalised token overlap between its TIGER street name and the winning way's name (approximating Nominatim's parent_place_id join), return that segment's ZIP. Ranked below way_postcodes (OSM postal boundaries stay authoritative), above the nearest-centroid fallback. Sidecar reads shared via sidecar_u32(). Added to the deployment manifests (configurations.template.json full mode, test-portal). Validated end-to-end: oceania byte-identical 122/122 with no sidecar; planet existing files byte-identical 122/122 + 129.6 MiB sidecar; same-PBF determinism 123/123, patch 251 B, verify 27/27; introduction-day patch 29.7 MB (was 80 MB pre-full-replace), verify 27/27; --continents build: sidecar only in TIGER-covered regions, north-america patch verify 27/27; day-over-day oceania verify 26/26 with baseline patch sizes; 327K-coord grid: zero collateral diffs; rural-33 ground truth: 13/20 previously-missing points now match live Nominatim exactly (1 adjacent-ZIP miss, 6 still missing from the single-nearest-segment approximation); geocoder-tests 99/99, cargo 51/51.
…ism noise floor resolved HANDOFF.md's "build non-determinism (remaining noise floor)" section described the pre-deterministic-ordering state; same-PBF planet chains are now byte-identical (251 B patch, verify 27/27) — mark it resolved. CODE_REVIEW.md: record the shipped B-1/B-2/B-4 fixes and update the rural-US postcode section from "NOT yet fixed" to the implemented TIGER per-segment ZIP design.
From a full PR-branch review (adversarially verified findings), byte-identical batch: - planet write vs continent filter race: apply_strategy2_remaps mutated shared ParsedData inside the planet async while continent filtering read the same arrays; the remap (and the GC_PREV_OUTPUT_ROOT setenv, itself thread-unsafe) now run once, synchronously, before the async launches - lost-wakeup in both bounded-concurrency throttles: state change now happens under the mutex before notify_one (deadlock risk when max_concurrent was 1) - "no shifts" strategy-2 log was tautological on reorder paths: the helper now receives the actual fast-path/reorder fact - TIGER/GeoNames loaders hard-fail (extraction, zero CSVs, zero rows) instead of silently building a US-less planet; quoted system() paths; extraction tmpdir cleaned up - checked stream writes everywhere an index file was written without a failure check (write_cell_index, strings_layout, sidecars, place_nodes, write_binary_file flush) - string pool throws past u32 offset space; DenseIndex counts and fails on node ids past MAX_NODE_ID (silent 0,0 coordinates before); cache version bumped with truncation checks + a loud incompleteness warning; PBF scan enforces blob size bounds and non-dense node groups fail loudly instead of dropping nodes; truncated UTF-8 no longer reads past the terminator in normalise_for_matching - dead code: geocoder-canonicalize tool (stale pre-v14 format) + its CMake target, canonicalize-only readers in patch_format.h, dead PatchHeader/Section/Encoding/opcodes + their test, write-only filtered_osm_ids and stored_merges, never-included relation_collector.h, dead ordered-mode reader branch, never-populated POI_PARENT_REMAP postcode leg (kept as literal 0 on the wire) - classify_poi extracted verbatim to poi_classify.h for unit testing Validated: oceania byte-identical 122/122 vs prior baseline; builder tests 98/98 (dead-opcode test removed with its subject).
…ntinent admin PIP
Three behaviour fixes from the PR-branch review (validated by a
categorized planet diff audit):
- qid_sitelinks.bin is packed 6-byte records (struct.pack('<IH', ...))
but was read with the padded 8-byte struct stride: record 0 parsed
correctly and every later record was garbage, silently zeroing
sitelink-based POI importance planet-wide. Parse the packed records
explicitly, hard-fail on size/sort violations.
- ring-assembly pass-2 backtracking leaked bt_used flags when a closed
path was rejected for self-intersection (try_close leaves path ways
marked on success): later seeds could never close rings through those
ways, losing admin rings.
- continent admin cell maps were filtered by the hand-written bboxes
while every other record class uses the Geofabrik boundary polygons:
zones the bboxes miss (Guam, Adak, the Azores) lost all admin data in
continent indexes. Cell membership now tests the polygon when
available, bbox fallback otherwise.
- watchdog: never dispatch when zero runs exist today (it raced the daily cron and produced duplicate multi-hour builds; retry-only now) - patch-verify: a missing per-variant result file (tool crash killed the verify subshell) now counts as FAIL instead of silently vanishing from the summary; geocoder-patch stderr goes to the per-variant diff log; file-count mismatches with a previous build present run the diff (added/removed optional files are real full-replace diffs — the old SKIP silently left e.g. a sidecar-introduction day unpatched); only a truly fresh build (no old output) skips - TIGER data missing/failed download now fails the build instead of silently shipping a US-less planet; wikidata latest.json fetch gets the same curl/jq guards as TIGER; replication diff fetches get --fail + gzip -t so a 404/truncation surfaces at fetch time - latest.json build/patch version stamps and EXPECTED_BUILD_VERSION are derived from configurations.template.json at runtime (were hardcoded 14/5/16; the stamps were two majors stale); template patch_version bumped 1 -> 2 for the GCPATCH v3 format - delete s3-cleanup.yml: superseded by geocoder-build's inline retention + latest.json writer; the two writers raced with different schemas (consumers only read .latest — verified) - run geocoder-tests in the build workflow before the planet build, and add a PR-triggered workflow running builder + server unit tests
- on-demand region downloads landed files in the manifest's nested
{region}/{mode}/ layout while Index::load reads a flat region dir —
the feature downloaded everything and then loaded nothing. Files now
land flat (later paths overwrite basenames, matching how server dirs
are assembled), and the region dir is keyed by build date so
size-only resume checks can't mix files from two builds
- format_postcode returned early on non-ASCII postcodes: every split
arm byte-indexes the uppercased string, which panics on a
non-char-boundary (reachable from raw addr:postcode tags)
- rate limiter map is evicted (entries older than yesterday) once it
exceeds 10k keys — with rate_by_ip every distinct client IP inserted
a key forever; RwLock guards recover from poisoning instead of
unwrap-panicking in handlers
- interpolation housenumber math: segment weights now use real lengths
(accumulating squared lengths skewed the along-way fraction), and the
even/odd snap computes in signed space clamped to the range
(descending ranges saturated the negative offset to zero); extracted
as a pure tested function
- corrupt-index bounds: read_entries_fb bounds the payload against EOF;
query_geo's street/interp lanes check record ids against file length
and node ranges against the node file before the unsafe slice reads
(node_count 0 previously underflowed the segment loop)
52/52 unit tests; grid parity run vs baseline follows with the planet
validation cycle.
- REQUIRE aborts the current test on failure (runner catches) — the six ring checks that indexed rings[0] after a failed size CHECK were UB; the runner also catches stray exceptions instead of dying mid-suite - lock in parity-critical pure functions that had zero unit coverage: the full is_included_highway accept/reject sets + footway subtype rules + the no-access/tunnel-filtering behaviour; ring_has_self_intersection in both the brute-force and sweep-line branches; classify_poi table tests (lodging/hut folds, tourism priority, highway flag) - new ring-assembly tests: reversed-way stitch, backtrack-after-dead-end (pairs with the bt_used leak fix), and an explicit lock-in that a single PRE-CLOSED way bypasses the self-intersection check (pass-2 only) — documented, not changed - the interpolation letter-housenumber test seeded end_number=0 and asserted 0: it couldn't fail; seed a sentinel so the resolver's write is actually observed - stride sentinel list gains 28/32/36 (current AddrPoint/PoiRecord) - doc corrections verified against the code: admin_osm_ids packing (closed ways have stable OSM_WAY ids since #105, not 0), continent_filter DOES preserve way_osm_ids now, ENTRY_CORRECTION records carry an 8-byte S2 cell id (not a 4-byte array index), CHECK_EQ prints expressions not values; types.h gains its missing <string> include 109/109 tests (474 checks).
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: dtolnay/rust-toolchain@stable |
Check failure
Code scanning / zizmor
unpinned action reference: action is not pinned to a hash (required by blanket policy) Error
| name: Server unit tests | ||
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
Check failure
Code scanning / zizmor
unpinned action reference: action is not pinned to a hash (required by blanket policy) Error
| name: Builder unit tests | ||
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
Check failure
Code scanning / zizmor
unpinned action reference: action is not pinned to a hash (required by blanket policy) Error
| name: Server unit tests | ||
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
Check warning
Code scanning / zizmor
credential persistence through GitHub Actions artifacts: does not set persist-credentials: false Warning
| name: Builder unit tests | ||
| runs-on: ubuntu-24.04 | ||
| steps: | ||
| - uses: actions/checkout@v4 |
Check warning
Code scanning / zizmor
credential persistence through GitHub Actions artifacts: does not set persist-credentials: false Warning
…own cell The INTERIOR_FLAG on a (cell, polygon) entry means "this cell is fully inside the polygon" — which proves containment only for query points inside THAT cell. find_pois / find_admin / find_nearest_poi_with_parent / debug_admin iterate the query cell plus its neighbors (for near-miss distance work) and honored the flag from neighbor entries too, claiming containment for points up to a full cell away: a query in Winterswijk (NL) returned "Naturpark Hohe Mark - Westmünsterland" (DE) as a contained landmark at 0.0 m because a neighboring cell across the border sat fully inside the park. Scope the flag to c == cell at all four sites. No true containment is lost: a polygon containing the query always has the query's own cell in its covering. Adds an examples/poi_debug tool that dumps per-cell entry flags + decoded geometry for a named POI at a point. Validated: 6,947 of 327,593 grid points change (all in the false- containment class — phantom landmark/admin claims from adjacent large polygons); spot-checked changed points against live Nominatim (Blacks Point NZ, Ōmokoroa NZ) — the new outputs match Nominatim, the old ones did not. 52/52 unit tests; the reported point (51.94431, 6.72012) now returns the plain Winterswijk address with no landmark.
…e centroids postcode_external_cc was keyed by interned string offsets but never remapped when the pool is partitioned into tiers, so write-time lookups with canonical ids mostly missed and occasionally false-hit a stale key, flipping ~250 centroid country codes between otherwise-identical builds (same-pbf patch grew 251 B -> 6 MB via strategy-2 identity churn). - remap postcode_external_cc alongside postcode_accum (planet tiers and continent flat pools) - accumulate postcode sums as 1e7-scaled integers: contributions arrive from dynamically-scheduled parse workers, so double sums wobbled in the last ulp with merge order - make country_code_at_point selection order-independent (smallest containing polygon area, ties by lowest country code) instead of first-in-vector - spatially filter postcode_accum in continent extracts: with the cc map fixed, geonames entries no longer depend on the geometric check that used to drop out-of-continent entries (~4.5M global centroids would otherwise ship in every continent)
find_linked_place step 4 used a +/-2 window on rank_search with the default level mapping; nominatim requires the node address rank to EQUAL the boundary's per-country rank_address. nl gemeentes (al8 -> rank 14) were linking their rank-16 town nodes and surfacing as towns instead of municipalities (winterswijk). - admin_rank_config.h: per-country admin_level -> rank_address table mirroring server/admin_levels.json (must stay in sync) - equality condition on address rank, boundary country resolved from the polygon centroid when untagged - exclude al11 from linking: those slots hold boundary=postal_code polygons, which nominatim never links - hide linked place nodes (nominatim linked_place_id): nodes claimed by label/wikidata links (relations and closed ways) and by step-4 name matches get name_id=NO_DATA, hiding them from all results; a claimed node is also excluded from later name matching (286k nodes on planet)
… slots - skip iso-less admin_level-2 polygons in find_admin, debug_admin and find_admin_from_chain: taiwan's 12nm maritime boundary closed way (maritime=yes, no iso) tied the real country polygon on area and could win by entry order; nominatim's country always comes from the country_code table so it is immune - suppress region when it duplicates country (kingdom of the netherlands al2 vs country al3, both named nederland) - address_havelevel rank 16: when the admin chain already yields a city/town/village boundary, skip the place-node fallback for those slots -- with linked city nodes now hidden, the town slot latched onto a far-away town across a district boundary (nelson nz picked richmond) and the fuzzy-area cascade then rejected the correct suburb/quarter
…t overshoot build_merge_seq re-anchors its cursor by hash-jumping to the smallest in-window new record with matching content. with duplicate record content (unnamed 2-node ways, shared strings after remap, zeroed tombstone slots) the smallest match past the cursor can be a LATER duplicate than the record's true counterpart; jumping to it starts a cascade where following records' true matches fall behind the forward-only window and drop to DELETE one by one until the cursor self-heals. on a chained 11-day planet pair (old side carried 39K tombstones) the episodes re-serialized 38.8M unchanged addr records and 31.7M ways -- a 2.79 GB patch for ~600 MB of real change. - only accept a far jump when the NEXT old record also finds its counterpart within 16 records after the target (tolerant to interleaved inserts); otherwise emit a single DELETE, which costs one record instead of an episode: patch 2.79 GB -> 597 MiB (-79%), addr merge-seq 1.27 GB -> 85 MB - exclude zeroed tombstone records from the jump index -- dead slots need no anchoring and every old tombstone hash-matches every still-empty new slot - pair content-identical duplicates positionally (FIFO) in the offset fixups instead of unordered_multimap find/erase, which pairs an arbitrary duplicate dump-pair 28-day patch unchanged (552->555 MB), content-identical rewrite sanity unchanged (7.6 MB), both windows patch-apply verified 27/27 byte-identical, 109/109 tests
the query path was already memory-light (everything mmap'd with MADV_RANDOM: ~10 MiB anonymous heap on a full planet index, all other resident pages reclaimable cache). validated under hard cgroup/docker limits: stable serving with correct results inside a 32 MiB container, ~5-15 ms warm queries, cold-cache planet bursts fine at 64 MiB. three things were not ready and are fixed here: - the dataset downloader buffered the whole compressed body AND the whole decompressed output in ram (multi-GiB peaks on planet files) -- guaranteed oom under a small container limit. now streams http -> zstd decode -> incremental sha256 -> tmp file, ram bounded by a few network chunks; failed downloads clean up their tmp file. covered by a streaming roundtrip test - tokio spawned one worker per core (65 threads on a 64-core host). pool now defaults to min(cores, 8), override with GEOCODER_WORKER_THREADS - no health endpoint for container orchestration: GET /health returns 200 with the loaded-region count once serving
- dockerfile HEALTHCHECK on /health - compose example with mem_limit + healthcheck + worker-threads env - readme section documenting the memory envelope and recommended container limits
…idecars keyless requests are accepted (rate limiting disabled) when the env flag is set — for deployments where the server is only reachable on a container-internal network, e.g. as an immich companion service. keyed auth still works unchanged; without the flag behaviour is identical.
immich keeps container count minimal — replace the sidecar proposal with a napi-rs native module (sharp-style prebuilds; lib.rs is already http/tokio-free) and a vendored-binary child process (exiftool-vendored pattern) as fallback/PoC shape
…embedding Geocoder.load(dir) mmaps the index (1.2 ms, +1.5 MB rss); reverseGeocode(lat, lng) runs as a napi AsyncTask on the libuv pool so cold page faults never block the event loop (max 3 ms gap during a fully cold 5,000-query burst); reverseGeocodeSync for callers that prefer it (0.42 ms warm). returns the address JSON. validated against the 1.3 GiB planet admin dataset under node v18 — v8 heap untouched, rss growth is reclaimable page cache only
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Complete geocoder system:
GitHub Actions workflow (geocoder-build.yml):
Migrated from traccar-geocoder with full validation history.