[geoloc] Handle Nominatim 429 rate limits with throttle and retry

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)

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")
+            )

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

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)

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", "")