From de84e191dfff7e6e0b920ea8a00df2b03be35777 Mon Sep 17 00:00:00 2001 From: Colin Powell Date: Fri, 31 Jul 2026 22:31:13 -0400 Subject: [PATCH] [geoloc] Handle Nominatim 429 rate limits with throttle and retry --- PROJECT.org | 48 +++- tests/locations_tests/__init__.py | 0 tests/locations_tests/test_reverse_geocode.py | 223 ++++++++++++++++++ vrobbler.conf.example | 5 + vrobbler/apps/locations/admin.py | 22 +- .../commands/reverse_geocode_missing.py | 51 ++++ vrobbler/apps/locations/utils.py | 75 +++++- vrobbler/apps/scrobbles/tasks.py | 48 +++- vrobbler/settings.py | 6 + 9 files changed, 450 insertions(+), 28 deletions(-) create mode 100644 tests/locations_tests/__init__.py create mode 100644 tests/locations_tests/test_reverse_geocode.py create mode 100644 vrobbler/apps/locations/management/commands/reverse_geocode_missing.py diff --git a/PROJECT.org b/PROJECT.org index 954c880..9e9a1b6 100644 --- a/PROJECT.org +++ b/PROJECT.org @@ -185,7 +185,7 @@ events). - Timestamp picker (default to now) - Optional: barcode scanning for food/drinks via CameraX -* Backlog [0/24] :vrobbler:project:personal: +* Backlog [1/26] :vrobbler:project:personal: ** TODO [#C] After transition to linux add curl_cffi as webpage scrapper again :webpages:metadata: ** TODO [#C] Create small utility to clean up tracks scrobbled with wonky playback times :bug:music:scrobbles: :PROPERTIES: @@ -698,6 +698,52 @@ The Edit log form should have from top to bottom: *** Description ** TODO [#A] Add trends tests for concurrent trends :trends:tests:concurrent: +** DONE [#A] Update agent flow to use OpenRouter and select model :agents: +:PROPERTIES: +:ID: 70f753a8-b320-4f35-1dcd-9f0d4f1c707b +:END: + +*** Description + +Currently the agent session is limited to only Google Gemini 3.5. We should +instead do something like how Foods works, where when a user enters a prompt +the next page should allow choosing one of the free models pulled from OR. + +From that point on, the scrobbles is of that agent model and the response should +come from there, with any susequent prompts going to thes ame model (with +context of past prompts and response). + +** TODO [#B] Mood scrobble page with "Check-in complete!" should ask for context :moods: + +*** Description + +Currently when scrobbling a mood, the finish page just says "complete!" Instead, +that page should be a simple form with a textarea in it with a request for +context. Saving that form should save the context as a note on the scrobble. + +* Version 63.3 [1/1] +** DONE [#A] Fix overuse of OSM API on reverse geocodes :bug:geolocations: +:PROPERTIES: +:ID: f8be6925-7d67-4ee4-9321-c570b4888f14 +:END: + +*** Description + +Nominatim rate limits (HTTP 429) were permanently dropping reverse geocodes +because the task swallowed the error and never retried, and nothing enforced +Nominatim's 1 req/sec policy. This adds a typed NominatimRateLimited exception, +a cross-worker redis throttle enforcing 1 req/sec, and Celery retry honoring +Retry-After with exponential backoff. Also added a reverse_geocode_missing +management command and switched the admin action to enqueue tasks. + +**** Tasks +- [X] Add NominatimRateLimited exception and redis throttle to locations/utils.py +- [X] Retry reverse_geocode_geolocation with Retry-After / exponential backoff +- [X] Add reverse_geocode_missing management command +- [X] Admin action enqueues tasks instead of blocking with a sleep +- [X] Add GEOLOC_REVERSE_GEOCODE_RATE / GEOLOC_REVERSE_GEOCODE_MAX_RETRIES settings +- [X] Write tests + * Version 63.2 [1/1] ** DONE [#A] Swap around AI API key for Google so Youtube still works :bug:agents: :PROPERTIES: diff --git a/tests/locations_tests/__init__.py b/tests/locations_tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/locations_tests/test_reverse_geocode.py b/tests/locations_tests/test_reverse_geocode.py new file mode 100644 index 0000000..2a12025 --- /dev/null +++ b/tests/locations_tests/test_reverse_geocode.py @@ -0,0 +1,223 @@ +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +import requests +from celery.exceptions import Retry +from locations import utils as locations_utils +from locations.models import GeoLocation +from scrobbles.tasks import reverse_geocode_geolocation + +LAT = 44.3652 +LON = -70.5057 + +ADDRESS = { + "road": "Main St", + "city": "Springfield", + "state": "ME", + "postcode": "01101", + "country": "USA", +} + +real_throttle = locations_utils._geocode_throttle + + +@pytest.fixture(autouse=True) +def _no_real_geocode_requests(): + with patch("locations.utils._geocode_throttle"): + yield + + +def _mock_response(status_code=200, json_data=None, headers=None): + mock_resp = MagicMock() + mock_resp.status_code = status_code + mock_resp.headers = headers or {} + if json_data is not None: + mock_resp.json.return_value = json_data + if status_code >= 400: + mock_resp.raise_for_status.side_effect = requests.HTTPError( + f"{status_code} Client Error", response=mock_resp + ) + return mock_resp + + +# --- reverse_geocode util --- + + +def test_reverse_geocode_success(): + with patch( + "locations.utils.requests.get", + return_value=_mock_response(json_data={"address": ADDRESS}), + ): + result = locations_utils.reverse_geocode(LAT, LON) + + assert result == { + "street": "Main St", + "city": "Springfield", + "state_province": "ME", + "postal_code": "01101", + "country": "USA", + } + + +def test_reverse_geocode_429_raises_rate_limited(): + mock_resp = _mock_response(status_code=429, headers={"Retry-After": "5"}) + with patch("locations.utils.requests.get", return_value=mock_resp): + with pytest.raises(locations_utils.NominatimRateLimited) as excinfo: + locations_utils.reverse_geocode(LAT, LON) + + assert excinfo.value.retry_after == 5 + + +def test_reverse_geocode_429_without_retry_after(): + mock_resp = _mock_response(status_code=429, headers={}) + with patch("locations.utils.requests.get", return_value=mock_resp): + with pytest.raises(locations_utils.NominatimRateLimited) as excinfo: + locations_utils.reverse_geocode(LAT, LON) + + assert excinfo.value.retry_after is None + + +def test_reverse_geocode_other_http_error_returns_none(): + mock_resp = _mock_response(status_code=500) + with patch("locations.utils.requests.get", return_value=mock_resp): + assert locations_utils.reverse_geocode(LAT, LON) is None + + +def test_reverse_geocode_connection_error_returns_none(): + with patch( + "locations.utils.requests.get", + side_effect=requests.ConnectionError("connection refused"), + ): + assert locations_utils.reverse_geocode(LAT, LON) is None + + +# --- throttle --- + + +class _FakeRedis: + def __init__(self, count): + self.count = count + + def incr(self, key): + self.count += 1 + return self.count + + def expire(self, key, ttl): + return True + + +@patch("locations.utils._geocode_throttle", real_throttle) +@patch("locations.utils._get_redis_client") +def test_geocode_throttle_raises_when_budget_exhausted(mock_client, settings): + settings.REDIS_URL = "redis://localhost:6379/0" + settings.GEOLOC_REVERSE_GEOCODE_RATE = 1 + mock_client.return_value = _FakeRedis(count=1) + + with pytest.raises(locations_utils.NominatimRateLimited) as excinfo: + locations_utils._geocode_throttle() + + assert excinfo.value.retry_after == 1 + + +@patch("locations.utils._geocode_throttle", real_throttle) +@patch("locations.utils._get_redis_client") +def test_geocode_throttle_passes_when_under_budget(mock_client, settings): + settings.REDIS_URL = "redis://localhost:6379/0" + settings.GEOLOC_REVERSE_GEOCODE_RATE = 1 + mock_client.return_value = _FakeRedis(count=0) + + assert locations_utils._geocode_throttle() is None + + +@patch("locations.utils._geocode_throttle", real_throttle) +def test_geocode_throttle_disabled_without_redis_url(settings): + settings.REDIS_URL = "" + settings.GEOLOC_REVERSE_GEOCODE_RATE = 1 + + assert locations_utils._geocode_throttle() is None + + +# --- celery task --- + + +@pytest.mark.django_db +def test_task_success_sets_postal_code(): + location = GeoLocation.objects.create(lat=LAT, lon=LON) + with patch( + "locations.utils.requests.get", + return_value=_mock_response(json_data={"address": ADDRESS}), + ): + reverse_geocode_geolocation.run(location.id) + + location.refresh_from_db() + assert location.postal_code == "01101" + assert location.city == "Springfield" + + +@pytest.mark.django_db +def test_task_retries_with_retry_after(settings): + settings.CELERY_TASK_ALWAYS_EAGER = False + location = GeoLocation.objects.create(lat=LAT, lon=LON) + mock_resp = _mock_response(status_code=429, headers={"Retry-After": "30"}) + + mock_retry = MagicMock(side_effect=Retry()) + fake_self = SimpleNamespace(request=SimpleNamespace(retries=0), retry=mock_retry) + task = reverse_geocode_geolocation + with patch("locations.utils.requests.get", return_value=mock_resp): + with pytest.raises(Retry): + task.run.__func__(fake_self, location.id) + + mock_retry.assert_called_once() + countdown = mock_retry.call_args.kwargs["countdown"] + assert countdown == 30 + location.refresh_from_db() + assert location.postal_code is None + + +@pytest.mark.django_db +def test_task_retry_backoff_without_retry_after(settings): + settings.CELERY_TASK_ALWAYS_EAGER = False + location = GeoLocation.objects.create(lat=LAT, lon=LON) + mock_resp = _mock_response(status_code=429, headers={}) + + mock_retry = MagicMock(side_effect=Retry()) + fake_self = SimpleNamespace(request=SimpleNamespace(retries=2), retry=mock_retry) + task = reverse_geocode_geolocation + with patch("locations.utils.requests.get", return_value=mock_resp): + with pytest.raises(Retry): + task.run.__func__(fake_self, location.id) + + countdown = mock_retry.call_args.kwargs["countdown"] + assert countdown >= 240 + assert countdown <= 250 + + +@pytest.mark.django_db +def test_task_gives_up_after_max_retries(settings): + settings.CELERY_TASK_ALWAYS_EAGER = False + settings.GEOLOC_REVERSE_GEOCODE_MAX_RETRIES = 3 + location = GeoLocation.objects.create(lat=LAT, lon=LON) + mock_resp = _mock_response(status_code=429, headers={}) + + mock_retry = MagicMock(side_effect=Retry()) + fake_self = SimpleNamespace(request=SimpleNamespace(retries=3), retry=mock_retry) + task = reverse_geocode_geolocation + with patch("locations.utils.requests.get", return_value=mock_resp): + task.run.__func__(fake_self, location.id) + + mock_retry.assert_not_called() + location.refresh_from_db() + assert location.postal_code is None + + +@pytest.mark.django_db +def test_task_skips_when_postal_code_set(): + location = GeoLocation.objects.create( + lat=LAT, lon=LON, postal_code="01101", city="Springfield" + ) + with patch( + "locations.utils.requests.get", + side_effect=AssertionError("should not hit Nominatim"), + ): + reverse_geocode_geolocation.run(location.id) diff --git a/vrobbler.conf.example b/vrobbler.conf.example index db25078..b96300f 100644 --- a/vrobbler.conf.example +++ b/vrobbler.conf.example @@ -28,6 +28,11 @@ VROBBLER_GOOGLE_AI_API_KEY="" VROBBLER_LICHESS_API_KEY = "" VROBBLER_FASTCORK_API_KEY="fc_" +# Geolocation reverse geocoding (Nominatim). Default rate limit is 1 request +# per second to comply with Nominatim's usage policy. +# VROBBLER_GEOLOC_REVERSE_GEOCODE_RATE="1" +# VROBBLER_GEOLOC_REVERSE_GEOCODE_MAX_RETRIES="8" + # Storages # VROBBLER_DATABASE_URL="postgres://USER:PASSWORD@HOST:PORT/NAME" # VROBBLER_REDIS_URL="redis://:PASS@HOST:6379/0" diff --git a/vrobbler/apps/locations/admin.py b/vrobbler/apps/locations/admin.py index 51cdbbc..f77bf54 100644 --- a/vrobbler/apps/locations/admin.py +++ b/vrobbler/apps/locations/admin.py @@ -1,11 +1,8 @@ -import time - from django.contrib import admin from django.http import HttpRequest - from locations.models import GeoLocation - from scrobbles.admin import ScrobbleInline +from scrobbles.tasks import reverse_geocode_geolocation @admin.register(GeoLocation) @@ -30,16 +27,9 @@ class GeoLocationAdmin(admin.ModelAdmin): @admin.action(description="Reverse geocode selected locations") def reverse_geocode_selected(self, request: HttpRequest, queryset): - updated = 0 - errors = 0 - for i, location in enumerate(queryset.iterator()): - if location.reverse_geocode(): - updated += 1 - else: - errors += 1 - if i < queryset.count() - 1: - time.sleep(1.1) - msg = f"Reverse geocoded {updated} locations" - if errors: - msg += f", {errors} failed" + enqueued = 0 + for location in queryset.filter(postal_code__isnull=True).iterator(): + reverse_geocode_geolocation.delay(location.id) + enqueued += 1 + msg = f"Enqueued reverse geocoding for {enqueued} locations" self.message_user(request, msg) diff --git a/vrobbler/apps/locations/management/commands/reverse_geocode_missing.py b/vrobbler/apps/locations/management/commands/reverse_geocode_missing.py new file mode 100644 index 0000000..6b797d9 --- /dev/null +++ b/vrobbler/apps/locations/management/commands/reverse_geocode_missing.py @@ -0,0 +1,51 @@ +from django.core.management.base import BaseCommand +from locations.models import GeoLocation +from scrobbles.tasks import reverse_geocode_geolocation + + +class Command(BaseCommand): + help = "Enqueue reverse geocoding for GeoLocations missing a postal code" + + def add_arguments(self, parser): + parser.add_argument( + "--limit", + type=int, + default=0, + help="Only enqueue the first N locations (default: all)", + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Show what would be enqueued without enqueueing", + ) + + def handle(self, *args, **options): + limit = options["limit"] + dry_run = options["dry_run"] + + queryset = GeoLocation.objects.filter(postal_code__isnull=True).order_by( + "-created" + ) + if limit: + queryset = queryset[:limit] + + total = queryset.count() + self.stdout.write(f"Found {total} GeoLocations missing a postal code") + + enqueued = 0 + for location in queryset.iterator(): + if dry_run: + self.stdout.write( + f"Would enqueue geo_location {location.id} " + f"({location.lat}, {location.lon})" + ) + else: + reverse_geocode_geolocation.delay(location.id) + enqueued += 1 + + if dry_run: + self.stdout.write(self.style.WARNING("Dry run - no tasks enqueued")) + else: + self.stdout.write( + self.style.SUCCESS(f"Enqueued {enqueued} reverse geocode tasks") + ) diff --git a/vrobbler/apps/locations/utils.py b/vrobbler/apps/locations/utils.py index a144457..2920bec 100644 --- a/vrobbler/apps/locations/utils.py +++ b/vrobbler/apps/locations/utils.py @@ -1,9 +1,12 @@ import logging +import time import xml.etree.ElementTree as ET from typing import Optional from urllib.parse import urlencode +import redis import requests +from django.conf import settings from django.utils import timezone from scrobbles.models import Scrobble @@ -32,7 +35,7 @@ ACTIVITY_MAPPING = { def haversine_distance(lat1: float, lon1: float, lat2: float, lon2: float) -> float: """Calculate the great-circle distance between two points in meters.""" - from math import radians, cos, sin, asin, sqrt + from math import asin, cos, radians, sin, sqrt lat1, lon1, lat2, lon2 = map(radians, [lat1, lon1, lat2, lon2]) dlat = lat2 - lat1 @@ -205,13 +208,78 @@ NOMINATIM_URL = "https://nominatim.openstreetmap.org/reverse" USER_AGENT = "Vrobbler/1.0 (https://github.com/secstate/vrobbler)" +_redis_client = None + + +class NominatimRateLimited(RuntimeError): + """Raised when Nominatim returns HTTP 429 or the local rate budget is exhausted. + + `retry_after` is the number of seconds to wait before retrying, or None to + fall back to exponential backoff. + """ + + def __init__(self, retry_after: Optional[int] = None): + self.retry_after = retry_after + super().__init__( + "Nominatim rate limit exceeded" + + (f"; retry after {retry_after}s" if retry_after else "") + ) + + +def _get_redis_client() -> Optional[redis.Redis]: + global _redis_client + if _redis_client is None and settings.REDIS_URL: + _redis_client = redis.from_url(settings.REDIS_URL) + return _redis_client + + +def _geocode_throttle() -> None: + """Enforce a global (cross-worker) rate limit for Nominatim requests. + + Uses a fixed one-second window in redis so multiple Celery workers share a + single budget. Raises NominatimRateLimited when the budget is exhausted. + Disabled when no REDIS_URL is configured, and degrades gracefully on redis + errors so geocoding never breaks because the rate limiter is down. + """ + rate = settings.GEOLOC_REVERSE_GEOCODE_RATE + if rate <= 0 or not settings.REDIS_URL: + return + key = f"vrobbler:nominatim:{int(time.time())}" + try: + client = _get_redis_client() + if client is None: + return + count = client.incr(key) + if count == 1: + client.expire(key, 2) + if count > rate: + raise NominatimRateLimited(1) + except NominatimRateLimited: + raise + except Exception as e: + logger.warning("Failed to enforce Nominatim rate limit: %s", e) + + +def _parse_retry_after(response) -> Optional[int]: + header = response.headers.get("Retry-After") + if not header: + return None + try: + return max(1, int(float(header))) + except (TypeError, ValueError): + return None + def reverse_geocode(lat: float, lon: float) -> Optional[dict]: """Reverse geocode lat/lon to an address using Nominatim. Returns a dict with address fields, or None on failure. + Raises NominatimRateLimited when Nominatim returns HTTP 429 or the local + rate budget is exhausted, so callers can retry with backoff. Nominatim usage policy: max 1 request per second. """ + _geocode_throttle() + params = { "lat": lat, "lon": lon, @@ -221,6 +289,11 @@ def reverse_geocode(lat: float, lon: float) -> Optional[dict]: try: resp = requests.get(NOMINATIM_URL, params=params, headers=headers, timeout=10) resp.raise_for_status() + except requests.HTTPError as e: + if e.response is not None and e.response.status_code == 429: + raise NominatimRateLimited(_parse_retry_after(e.response)) from e + logger.warning("Failed to reverse geocode %s,%s: %s", lat, lon, e) + return None except requests.RequestException as e: logger.warning("Failed to reverse geocode %s,%s: %s", lat, lon, e) return None diff --git a/vrobbler/apps/scrobbles/tasks.py b/vrobbler/apps/scrobbles/tasks.py index 38ae7ef..f0ffc81 100644 --- a/vrobbler/apps/scrobbles/tasks.py +++ b/vrobbler/apps/scrobbles/tasks.py @@ -1,4 +1,5 @@ import logging +import random from datetime import datetime, timedelta from celery import shared_task @@ -828,9 +829,10 @@ def add_scrobble_to_mopidy_monthly_playlist(scrobble_id): add_track_to_mopidy_monthly_playlist(scrobble) -@shared_task -def reverse_geocode_geolocation(geo_location_id): +@shared_task(bind=True) +def reverse_geocode_geolocation(self, geo_location_id): from locations.models import GeoLocation + from locations.utils import NominatimRateLimited location = GeoLocation.objects.filter(id=geo_location_id).first() if not location: @@ -853,14 +855,40 @@ def reverse_geocode_geolocation(geo_location_id): location.lat, location.lon, ) - if location.reverse_geocode(): - logger.info( - "Reverse geocode succeeded for geo_location %s: %s", - geo_location_id, - location.display_address, - ) - else: + try: + if location.reverse_geocode(): + logger.info( + "Reverse geocode succeeded for geo_location %s: %s", + geo_location_id, + location.display_address, + ) + else: + logger.warning( + "Reverse geocode failed for geo_location %s", + geo_location_id, + ) + except NominatimRateLimited as e: + max_retries = settings.GEOLOC_REVERSE_GEOCODE_MAX_RETRIES + if self.request.retries >= max_retries: + logger.warning( + "Giving up reverse geocoding geo_location %s after %s " + "rate-limited attempts", + geo_location_id, + max_retries, + ) + return + countdown = e.retry_after + if not countdown: + countdown = 60 * (2**self.request.retries) + random.randint(0, 10) logger.warning( - "Reverse geocode failed for geo_location %s", + "Reverse geocode rate limited for geo_location %s, retrying in %ss", geo_location_id, + countdown, ) + if settings.CELERY_TASK_ALWAYS_EAGER: + logger.info( + "Skipping retry of geo_location %s in eager mode", + geo_location_id, + ) + return + raise self.retry(exc=e, countdown=countdown) diff --git a/vrobbler/settings.py b/vrobbler/settings.py index 0bd74e1..50942dc 100644 --- a/vrobbler/settings.py +++ b/vrobbler/settings.py @@ -75,6 +75,12 @@ DB_BACKUP_NTFY_URL = os.getenv( ) GEOLOC_ACCURACY = os.getenv("VROBBLER_GEOLOC_ACCURACY", 3) GEOLOC_PROXIMITY = os.getenv("VROBBLER_GEOLOC_PROXIMITY", "0.0001") +GEOLOC_REVERSE_GEOCODE_RATE = int( + os.getenv("VROBBLER_GEOLOC_REVERSE_GEOCODE_RATE", "1") +) +GEOLOC_REVERSE_GEOCODE_MAX_RETRIES = int( + os.getenv("VROBBLER_GEOLOC_REVERSE_GEOCODE_MAX_RETRIES", "8") +) POINTS_FOR_MOVEMENT_HISTORY = os.getenv("VROBBLER_POINTS_FOR_MOVEMENT_HISTORY", 3) TODOIST_CLIENT_ID = os.getenv("VROBBLER_TODOIST_CLIENT_ID", "") TODOIST_CLIENT_SECRET = os.getenv("VROBBLER_TODOIST_CLIENT_SECRET", "")