data/climate: fix geocode_nominatim's NameError on every call (P0, live) #51

Merged

admin_emi merged 1 commit from fix/geocode-nominatim-revgeo-lock into dev

2026-07-24 19:45:51 +00:00

admin_emi commented

2026-07-24 19:44:09 +00:00

Owner

Summary

geocode_nominatim referenced _REVGEO_LOCK, which no longer exists — it was removed when reverse geocoding moved from a bare lock to the dedicated worker/queue design, and the forward-geocode function was never updated to match. Every call raised NameError, surfacing as a 502 on /geocode's Nominatim-fallback path. This has been live on prod, beta, dev, main, and release for ~30 hours — found during a post-hoc audit of the recent Open-Meteo-exit/ERA5-lake/daemon batch, not by a report.

Blast radius: bare city names in the local GeoNames index still worked (so smoke checks passed), but every comma-qualified name ("Durham, NC"), postcode, neighbourhood, sub-1000-population village, or genuine local-index miss 502'd. Prod logs show real SSR traffic hitting this (Nørrebro, Shimokitazawa, SW1A 1AA). The three existing tests covering this path all monkeypatch geocode_nominatim itself, so none of them ever executed the broken body — 365 green tests, 100%-broken production path.

Fix

Routes forward-geocode jobs through the same worker/queue that already serializes reverse-geocode jobs, instead of reintroducing a second lock. Both job kinds are now drained by the one worker thread, so _revgeo_last still has exactly one writer (no lock needed — same invariant as before) and the shared ~1/sec Nominatim pacing the docstring always claimed is now actually enforced.

Deliberately not the naive fix (_REVGEO_LOCK = threading.Lock()): the worker mutates _revgeo_last with no lock by design (single-writer), so a second uncoordinated pacer would race it and could violate the documented Nominatim ≤1 req/s policy.

Tests

5 new tests exercising the real queue/worker plumbing (not a monkeypatch of geocode_nominatim), matching the existing reverse-geocode test style.
1 new test stubbing _request to run _fetch_geocode_forward's actual body — the function whose earlier version never once executed successfully in production.
Full suite: 386 passed, 7 skipped, 2 pre-existing unrelated failures (missing duckdb package in this local venv, nothing to do with geocoding).

Deploy

Targeting dev first for a beta smoke-test, then promoting to main/release promptly given severity.

## Summary `geocode_nominatim` referenced `_REVGEO_LOCK`, which no longer exists — it was removed when reverse geocoding moved from a bare lock to the dedicated worker/queue design, and the forward-geocode function was never updated to match. **Every call raised `NameError`**, surfacing as a 502 on `/geocode`'s Nominatim-fallback path. This has been live on prod, beta, dev, main, and release for ~30 hours — found during a post-hoc audit of the recent Open-Meteo-exit/ERA5-lake/daemon batch, not by a report. Blast radius: bare city names in the local GeoNames index still worked (so smoke checks passed), but every comma-qualified name ("Durham, NC"), postcode, neighbourhood, sub-1000-population village, or genuine local-index miss 502'd. Prod logs show real SSR traffic hitting this (`Nørrebro`, `Shimokitazawa`, `SW1A 1AA`). The three existing tests covering this path all monkeypatch `geocode_nominatim` itself, so none of them ever executed the broken body — 365 green tests, 100%-broken production path. ## Fix Routes forward-geocode jobs through the same worker/queue that already serializes reverse-geocode jobs, instead of reintroducing a second lock. Both job kinds are now drained by the one worker thread, so `_revgeo_last` still has exactly one writer (no lock needed — same invariant as before) and the shared ~1/sec Nominatim pacing the docstring always claimed is now actually enforced. Deliberately **not** the naive fix (`_REVGEO_LOCK = threading.Lock()`): the worker mutates `_revgeo_last` with no lock by design (single-writer), so a second uncoordinated pacer would race it and could violate the documented Nominatim ≤1 req/s policy. ## Tests - 5 new tests exercising the real queue/worker plumbing (not a monkeypatch of `geocode_nominatim`), matching the existing reverse-geocode test style. - 1 new test stubbing `_request` to run `_fetch_geocode_forward`'s actual body — the function whose earlier version never once executed successfully in production. - Full suite: 386 passed, 7 skipped, 2 pre-existing unrelated failures (missing `duckdb` package in this local venv, nothing to do with geocoding). ## Deploy Targeting `dev` first for a beta smoke-test, then promoting to `main`/`release` promptly given severity.

admin_emi added 1 commit 2026-07-24 19:44:11 +00:00

data/climate: fix geocode_nominatim's NameError on every call

PR build (required check) / changes (pull_request) Successful in 7s

Details

secrets-guard / encrypted (pull_request) Successful in 6s

Details

shell-lint / shellcheck (pull_request) Successful in 8s

Details

PR build (required check) / build-frontend (pull_request) Has been skipped

Details

PR build (required check) / validate-observability (pull_request) Has been skipped

Details

PR build (required check) / build-backend (pull_request) Successful in 1m9s

Details

PR build (required check) / gate (pull_request) Successful in 2s

Details

177b123495

geocode_nominatim referenced _REVGEO_LOCK, which was removed when
reverse geocoding moved from a bare lock to the dedicated worker/queue
design -- the forward-geocode function was never updated to match, so
every call raised NameError, degrading to a 502 on the frontend. Live
on prod, beta, dev, main, and release since the lock was removed
(~30h): every comma-qualified name, postcode, and non-cities1000 place
502'd, while the local GeoNames index kept bare city names working, so
the failure was invisible to simple smoke checks. The three existing
tests covering this path all monkeypatch geocode_nominatim itself, so
none of them ever executed the broken body.

Fixed by routing forward-geocode jobs through the same worker/queue
that already serializes reverse-geocode jobs, instead of reintroducing
a second lock -- both job kinds are now drained by the one worker
thread, so _revgeo_last still has exactly one writer and the shared
~1/sec Nominatim pacing the docstring always claimed is now actually
enforced, not just asserted in a comment.

Adds regression coverage that exercises the real queue/worker plumbing
(not a monkeypatch of geocode_nominatim) plus a stubbed-HTTP test of
_fetch_geocode_forward's own body, the function whose earlier version
never once executed successfully.