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

backend/data/climate.py

@@ -1009,10 +1009,13 @@ _REVGEO_CACHE: dict[str, str | None] = {}
 _REVGEO_MIN_INTERVAL = 1.1   # seconds between successive Nominatim reverse calls
 _revgeo_last = 0.0
 
-_REVGEO_QUEUE: "queue.Queue[tuple[float, float, str, concurrent.futures.Future]]" = queue.Queue()
+_REVGEO_QUEUE: "queue.Queue[tuple]" = queue.Queue()
 _REVGEO_WORKER_LOCK = threading.Lock()
 _revgeo_worker_started = False
 _REVGEO_WAIT_TIMEOUT = 10.0   # seconds a caller waits for ITS OWN request before giving up
+_GEOCODE_WAIT_TIMEOUT = 35.0  # forward /geocode is a deliberate user search, not a map-pan
+                              # enrichment -- worth a longer wait than reverse geocoding's,
+                              # covering the 30s per-request timeout plus queue wait.
 
 
 def reverse_geocode_cached(lat: float, lon: float) -> tuple[bool, str | None]:
@@ -1065,17 +1068,21 @@ def _fetch_revgeo_label(lat: float, lon: float) -> str | None:
 
 
 def _revgeo_worker() -> None:
-    """Drains _REVGEO_QUEUE one request at a time, forever. The sole caller of
-    _fetch_revgeo_label, so the ~1/sec pacing below is enforced just by doing the
-    work serially — no lock needed, since nothing else ever touches Nominatim.
-    Re-checks the cache before fetching (a request queued behind an identical one
-    is answered from what the earlier request just cached, no duplicate call),
-    and always finishes the fetch + persists the result even if the original
-    caller already gave up waiting (see reverse_geocode's timeout)."""
+    """Drains _REVGEO_QUEUE one job at a time, forever — both reverse ("revgeo")
+    and forward ("geocode") jobs, so the ~1/sec pacing below is enforced just by
+    doing the work serially on this one thread, no lock needed, since nothing
+    else ever touches Nominatim. Reverse jobs re-check the cache before fetching
+    (a request queued behind an identical one is answered from what the earlier
+    request just cached, no duplicate call) and always finish + persist even if
+    the original caller already gave up waiting (see reverse_geocode's timeout).
+    Forward jobs have no cache (see geocode_nominatim) but share the same pacer."""
     global _revgeo_last
     while True:
-        lat, lon, key, fut = _REVGEO_QUEUE.get()
+        job = _REVGEO_QUEUE.get()
+        kind = job[0]
         try:
+            if kind == "revgeo":
+                _, lat, lon, key, fut = job
                 found, label = reverse_geocode_cached(lat, lon)
                 if not found:
                     wait = _REVGEO_MIN_INTERVAL - (time.monotonic() - _revgeo_last)
@@ -1087,9 +1094,20 @@ def _revgeo_worker() -> None:
                     store.put_revgeo(key, label)   # survive restarts (a None label retries after its TTL)
                 if not fut.done():
                     fut.set_result(label)
+            else:  # "geocode"
+                _, name, count, fut = job
+                wait = _REVGEO_MIN_INTERVAL - (time.monotonic() - _revgeo_last)
+                if wait > 0:
+                    time.sleep(wait)
+                try:
+                    results = _fetch_geocode_forward(name, count)
+                finally:
+                    _revgeo_last = time.monotonic()
+                if not fut.done():
+                    fut.set_result(results)
         except Exception:  # noqa: BLE001 - never let a bad request kill the worker
             if not fut.done():
-                fut.set_result(None)
+                fut.set_result(None if kind == "revgeo" else [])
         finally:
             _REVGEO_QUEUE.task_done()
 
@@ -1129,33 +1147,19 @@ def reverse_geocode(lat: float, lon: float) -> str | None:
         return label
     _start_revgeo_worker()
     fut: "concurrent.futures.Future[str | None]" = concurrent.futures.Future()
-    _REVGEO_QUEUE.put((lat, lon, key, fut))
+    _REVGEO_QUEUE.put(("revgeo", lat, lon, key, fut))
     try:
         return fut.result(timeout=_REVGEO_WAIT_TIMEOUT)
     except concurrent.futures.TimeoutError:
         return None
 
 
-def geocode_nominatim(name: str, count: int = 5) -> list[dict]:
-    """Forward place-name lookup via OpenStreetMap Nominatim (keyless).
-
-    Replaces the former Open-Meteo geocoder. Nominatim covers the long tail the
-    local GeoNames index can't — neighbourhoods, postcodes, sub-1000-population
-    villages, and alternate/native-language spellings — so /geocode falls back to
-    it on a local miss. It carries no population, so results keep Nominatim's own
-    relevance order; name/admin/country map straight across.
-
-    Shares the reverse geocoder's lock and ~1/sec pacing: both hit the same host,
-    so serializing them together keeps total Nominatim traffic under the usage
-    policy. Only the low-volume /geocode miss path reaches here — autocomplete
-    (/suggest) is served purely from the local index and never calls out.
-    """
-    global _revgeo_last
-    with _REVGEO_LOCK:
-        wait = _REVGEO_MIN_INTERVAL - (time.monotonic() - _revgeo_last)
-        if wait > 0:
-            time.sleep(wait)
-        try:
+def _fetch_geocode_forward(name: str, count: int) -> list[dict]:
+    """The actual Nominatim forward-search HTTP call + result shaping. Called
+    ONLY from _revgeo_worker (never on a caller's thread), mirroring
+    _fetch_revgeo_label — but unlike that function, a bad response is allowed to
+    raise here; _revgeo_worker's own except clause turns it into an empty list,
+    same net effect without a second layer of exception-swallowing."""
     r = _request(
         "https://nominatim.openstreetmap.org/search",
         {"q": name, "format": "jsonv2", "addressdetails": 1,
@@ -1165,8 +1169,6 @@ def geocode_nominatim(name: str, count: int = 5) -> list[dict]:
         headers={"User-Agent": "Thermograph/0.1 (local weather grading app)"},
     )
     rows = r.json() or []
-        finally:
-            _revgeo_last = time.monotonic()
     out = []
     for g in rows:
         a = g.get("address", {}) or {}
@@ -1187,3 +1189,28 @@ def geocode_nominatim(name: str, count: int = 5) -> list[dict]:
             "population": None,   # Nominatim has no population; order is relevance-based
         })
     return out
+
+
+def geocode_nominatim(name: str, count: int = 5) -> list[dict]:
+    """Forward place-name lookup via OpenStreetMap Nominatim (keyless).
+
+    Replaces the former Open-Meteo geocoder. Nominatim covers the long tail the
+    local GeoNames index can't — neighbourhoods, postcodes, sub-1000-population
+    villages, and alternate/native-language spellings — so /geocode falls back to
+    it on a local miss. It carries no population, so results keep Nominatim's own
+    relevance order; name/admin/country map straight across.
+
+    Shares the reverse geocoder's worker thread and ~1/sec pacing: both hit the
+    same host, and both jobs are drained serially by the one thread in
+    _revgeo_worker, so there is exactly one writer of _revgeo_last — no lock
+    needed, same reasoning as reverse_geocode. Only the low-volume /geocode miss
+    path reaches here — autocomplete (/suggest) is served purely from the local
+    index and never calls out.
+    """
+    _start_revgeo_worker()
+    fut: "concurrent.futures.Future[list[dict]]" = concurrent.futures.Future()
+    _REVGEO_QUEUE.put(("geocode", name, count, fut))
+    try:
+        return fut.result(timeout=_GEOCODE_WAIT_TIMEOUT) or []
+    except concurrent.futures.TimeoutError:
+        return []

backend/tests/data/test_climate.py

@@ -615,3 +615,104 @@ def test_reverse_geocode_timeout_returns_none_without_blocking_the_caller(monkey
     elapsed = time_mod.monotonic() - t0
     assert label is None
     assert elapsed < 0.2   # returned near the wait timeout, not after the 0.3s fetch
+
+
+# --- forward geocode: shares the reverse-geocode worker, not a second lock -----
+# Regression coverage for the NameError _REVGEO_LOCK bug (geocode_nominatim
+# referenced a lock that was removed when reverse geocoding moved to the
+# worker/queue design, so every forward lookup 502'd in production). These
+# exercise the real queue/worker plumbing rather than mocking geocode_nominatim
+# itself away, so a reintroduced bare `with _REVGEO_LOCK:` or any other
+# not-actually-defined-name bug fails loudly here instead of shipping unseen.
+
+def test_geocode_nominatim_resolves_via_the_worker_thread(monkeypatch):
+    monkeypatch.setattr(
+        climate, "_fetch_geocode_forward",
+        lambda name, count: [{"name": name, "admin1": None, "country": "Testland",
+                               "country_code": "TL", "lat": 1.0, "lon": 2.0,
+                               "population": None}],
+    )
+    results = climate.geocode_nominatim("Nowheresville")
+    assert results[0]["name"] == "Nowheresville"
+    assert results[0]["country"] == "Testland"
+
+
+def test_geocode_nominatim_timeout_returns_empty_list(monkeypatch):
+    """Same shape as reverse_geocode's timeout test: a caller waits only up to
+    _GEOCODE_WAIT_TIMEOUT, not as long as the fetch itself takes."""
+    import time as time_mod
+    monkeypatch.setattr(climate, "_GEOCODE_WAIT_TIMEOUT", 0.05)
+
+    def slow_fetch(name, count):
+        time_mod.sleep(0.3)
+        return [{"name": "Too Slow"}]
+
+    monkeypatch.setattr(climate, "_fetch_geocode_forward", slow_fetch)
+    t0 = time_mod.monotonic()
+    results = climate.geocode_nominatim("anywhere")
+    elapsed = time_mod.monotonic() - t0
+    assert results == []
+    assert elapsed < 0.2
+
+
+def test_geocode_nominatim_a_bad_fetch_degrades_to_empty_list_not_a_crash(monkeypatch):
+    """_fetch_geocode_forward is allowed to raise (matches _fetch_revgeo_label's
+    contract loosely -- the worker's except clause is the actual safety net);
+    confirm a raising fetch never reaches the caller as an exception."""
+    def boom(name, count):
+        raise RuntimeError("Nominatim is down")
+    monkeypatch.setattr(climate, "_fetch_geocode_forward", boom)
+    assert climate.geocode_nominatim("anywhere") == []
+
+
+def test_geocode_nominatim_shares_the_reverse_geocode_pacer(monkeypatch):
+    """Forward and reverse jobs are drained by the SAME worker thread off the
+    SAME queue, so a forward call advances _revgeo_last exactly like a reverse
+    one does -- this is what makes a second lock unnecessary."""
+    monkeypatch.setattr(climate, "_revgeo_last", 0.0)
+    monkeypatch.setattr(climate, "_fetch_geocode_forward", lambda name, count: [])
+    before = climate._revgeo_last
+    climate.geocode_nominatim("anywhere")
+    assert climate._revgeo_last > before
+
+
+def test_fetch_geocode_forward_parses_nominatim_response(monkeypatch):
+    """Executes _fetch_geocode_forward's real body (the function the NameError
+    bug prevented from ever running) against a stubbed HTTP transport -- not a
+    monkeypatch of geocode_nominatim itself, unlike the API-layer tests."""
+    class _FakeResp:
+        def json(self):
+            return [
+                {"name": "West Seattle", "lat": "47.57", "lon": "-122.38",
+                 "display_name": "West Seattle, Seattle, King County, Washington, United States",
+                 "address": {"suburb": "West Seattle", "city": "Seattle",
+                             "state": "Washington", "country": "United States",
+                             "country_code": "us"}},
+                # No `name`, no recognized address component -- falls back to the
+                # head of display_name, exercising that branch too.
+                {"lat": "51.5", "lon": "-0.1",
+                 "display_name": "Some Unnamed Place, Greater London, England",
+                 "address": {"country": "United Kingdom", "country_code": "gb"}},
+            ]
+
+    captured = {}
+
+    def fake_request(url, params, timeout, *, phase, headers=None, attempts=climate.MAX_ATTEMPTS):
+        captured["url"] = url
+        captured["params"] = params
+        captured["phase"] = phase
+        return _FakeResp()
+
+    monkeypatch.setattr(climate, "_request", fake_request)
+    results = climate._fetch_geocode_forward("west seattle", 5)
+
+    assert captured["url"] == "https://nominatim.openstreetmap.org/search"
+    assert captured["params"]["q"] == "west seattle"
+    assert captured["phase"] == "geocode"
+
+    assert results[0] == {
+        "name": "West Seattle", "admin1": "Washington", "country": "United States",
+        "country_code": "US", "lat": 47.57, "lon": -122.38, "population": None,
+    }
+    assert results[1]["name"] == "Some Unnamed Place"
+    assert results[1]["country_code"] == "GB"