diff --git a/server/src/nest/README.md b/server/src/nest/README.md index 8ab160682..f31bba18e 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -101,9 +101,14 @@ tools, which never pass through an HTTP guard. In the five domains piloted for client's `X-Idempotency-Key` on mutations. It replaced the `applyIdempotency` middleware, which is deleted; this is the only implementation. - `common/` — the stateless helpers (`avatarUrl`, `conflictResult`, `demo`, - `passwordPolicy`, `timezoneService`, `cookie`, `rowShape`, `crypto/`). Free - functions, not providers: `db/migrations.ts` imports them from outside the - container and could not inject one. + `passwordPolicy`, `timezoneService`, `cookie`, `rowShape`, `geo`, `crypto/`). + Free functions, not providers: `db/migrations.ts` imports them from outside the + container and could not inject one. `geo.ts` holds the two haversines; there + were three implementations of the metre variant, differing by one clamp, and + the clamped form is the one kept — `asin` of a value a hair over 1 is NaN, and + a NaN distance silently fails every comparison it feeds instead of throwing. + `maps.helpers.ts` re-exports `haversineMetres` rather than defining it, because + callers and a test import it from there. - `auth/jwt-verify.ts` — `extractToken` + `verifyJwtAndLoadUser`, the canonical session check behind the four guards, the MCP bearer path and the file-download query token. Free functions for the same reason. @@ -119,6 +124,35 @@ tools, which never pass through an HTTP guard. In the five domains piloted for answer 404 to anonymous callers, so the addon check must beat the 401. - `common/demo-write.ts` — `isDemoWriteBlocked`, the demo-mode upload block. A function and not a guard on purpose; see the gotcha below. +- `geo/` — the ONE Nominatim client (#576). There used to be two: atlas + throttled to the published 1.1s and cached its answers, maps did neither, so + five interactive routes fired straight at the service while the atlas loops + politely waited and OSM saw a single instance ignoring its rate limit. Both + domains now go through `nominatimFetch`. + - The throttle cursor and the cache are **module state, not provider state**, + for the permissions-cache reason: the bridges build their collaborators with + `new`, outside the container, and a second copy of the cursor is the very bug + the fold removes. `GeoModule` is registered straight in AppModule and neither + `MapsModule` nor `AtlasModule` imports it — an import would claim an + injection edge that does not exist. + - `GeocodingService` owns exactly one thing: the cache-cleanup timer's + lifecycle, via `OnModuleInit`/`OnModuleDestroy`. Atlas used to start that + interval as an import-time side effect, documented as a parity exception. It + is not one any more. + - Two lanes over one cursor. Interactive calls (`/api/maps/search`, + `/autocomplete`, `/details`, `/reverse`, `/resolve-url`) take the next slot; + background calls (the atlas enrichment loops, which walk every uncached + place) yield to anything queued on a request path. + - The cache key carries the **query shape**, not coordinates alone. Atlas asks + at zoom 3 and zoom 8, maps at zoom 18; a coordinate-only key answered a + street lookup with a country. + - `AbortSignal.timeout` is built **after** the throttle wait. Built up front, + the 1.1s spent queueing would eat nearly half of the identity lookup's 2.5s + budget before the socket opens. + - `setGeoThrottleInterval(ms)` is a test seam, and `tests/setup.ts` zeroes it. + Nineteen maps cases drive these paths back to back with real timers; at the + real interval they slept nineteen seconds and covered nothing extra. A + function rather than a `NODE_ENV` check, for the reason app-config exists. ### Reaching the container from outside it diff --git a/server/src/nest/app.module.ts b/server/src/nest/app.module.ts index 65cdbc234..ae98ff7fc 100644 --- a/server/src/nest/app.module.ts +++ b/server/src/nest/app.module.ts @@ -13,6 +13,7 @@ import { AirportsModule } from './airports/airports.module'; import { ConfigModule } from './config/config.module'; import { SystemNoticesModule } from './system-notices/system-notices.module'; import { MapsModule } from './maps/maps.module'; +import { GeoModule } from './geo/geo.module'; import { PlaceEnrichmentModule } from './place-enrichment/place-enrichment.module'; import { CategoriesModule } from './categories/categories.module'; import { TagsModule } from './tags/tags.module'; @@ -65,7 +66,7 @@ import { IdempotencyInterceptor } from './common/idempotency.interceptor'; * migrated. */ @Module({ - imports: [AppConfigModule, DatabaseModule, RealtimeModule, McpModule.forRoot({ accessPolicy: trekMcpAccessPolicy, validateAccess: trekMcpValidateAccess }), HealthModule, WeatherModule, HelpModule, AirportsModule, ConfigModule, SystemNoticesModule, MapsModule, PlaceEnrichmentModule, CategoriesModule, TagsModule, NotificationsModule, AtlasModule, VacayModule, PackingModule, TodoModule, BudgetModule, ReservationsModule, DaysModule, DayNotesModule, AccommodationsModule, AssignmentsModule, PlacesModule, TripsModule, CollabModule, FilesModule, PhotosModule, MemoriesModule, AirtrailModule, JourneyModule, CollectionsModule, ShareModule, TripInviteModule, TransitModule, FeedsModule, SettingsModule, BackupModule, AuthModule, OidcModule, OauthModule, AdminModule, AddonsModule, AuditModule, PermissionsModule, PluginsModule, BookingImportModule, ReservationImportModule, LlmParseModule], + imports: [AppConfigModule, DatabaseModule, RealtimeModule, McpModule.forRoot({ accessPolicy: trekMcpAccessPolicy, validateAccess: trekMcpValidateAccess }), HealthModule, WeatherModule, HelpModule, AirportsModule, ConfigModule, SystemNoticesModule, GeoModule, MapsModule, PlaceEnrichmentModule, CategoriesModule, TagsModule, NotificationsModule, AtlasModule, VacayModule, PackingModule, TodoModule, BudgetModule, ReservationsModule, DaysModule, DayNotesModule, AccommodationsModule, AssignmentsModule, PlacesModule, TripsModule, CollabModule, FilesModule, PhotosModule, MemoriesModule, AirtrailModule, JourneyModule, CollectionsModule, ShareModule, TripInviteModule, TransitModule, FeedsModule, SettingsModule, BackupModule, AuthModule, OidcModule, OauthModule, AdminModule, AddonsModule, AuditModule, PermissionsModule, PluginsModule, BookingImportModule, ReservationImportModule, LlmParseModule], providers: [ // Default-deny: a route is authenticated unless it carries @Public() or // @OptionalAuth(), or declares its own @UseGuards chain. Protection used to diff --git a/server/src/nest/atlas/atlas-geo.ts b/server/src/nest/atlas/atlas-geo.ts index ade89d6d6..2ed65401e 100644 --- a/server/src/nest/atlas/atlas-geo.ts +++ b/server/src/nest/atlas/atlas-geo.ts @@ -3,6 +3,7 @@ import { Place } from '../../types'; import fs from 'fs'; import path from 'path'; import zlib from 'zlib'; +import { cacheKeyFor, getCached, nominatimFetch, setCached } from '../geo/nominatim.client'; // ── Pure geo machinery for the atlas domain ───────────────────────────────── // @@ -215,42 +216,6 @@ function buildAdmin1Store(): Promise> { }); } -// ── Geocode cache ─────────────────────────────────────────────────────────── - -const geocodeCache = new Map(); - -function roundKey(lat: number, lng: number): string { - return `${lat.toFixed(3)},${lng.toFixed(3)}`; -} - -function cacheKey(lat: number, lng: number): string { - return roundKey(lat, lng); -} - -export function getCached(lat: number, lng: number): string | null | undefined { - const key = cacheKey(lat, lng); - if (geocodeCache.has(key)) return geocodeCache.get(key)!; - return undefined; -} - -export function setCache(lat: number, lng: number, code: string | null): void { - geocodeCache.set(cacheKey(lat, lng), code); -} - -// Periodically trim the cache so it doesn't grow unbounded. -// Import-time side effect preserved verbatim from the legacy module (parity -// exception, same class as audit-log.logger.ts): unref'd so it never holds the -// process open. -const CACHE_MAX = 50_000; -const CACHE_CLEANUP_MS = 10 * 60 * 1000; -setInterval(() => { - if (geocodeCache.size > CACHE_MAX) { - const keys = [...geocodeCache.keys()]; - const toDelete = keys.slice(0, keys.length - CACHE_MAX); - for (const k of toDelete) geocodeCache.delete(k); - } -}, CACHE_CLEANUP_MS).unref(); - // ── Bounding-box lookup tables ────────────────────────────────────────────── // Territories that have their own ISO code but no admin0 polygon in the bundle. @@ -519,31 +484,20 @@ export const NAME_TO_CODE: Record = { // ── Geocoding helpers ─────────────────────────────────────────────────────── -let lastNominatimCall = 0; - -// Shared throttle: enforces ≥1.1s between any Nominatim request, across all callers. -async function throttleNominatim() { - const elapsed = Date.now() - lastNominatimCall; - if (elapsed < 1100) await new Promise((r) => setTimeout(r, 1100 - elapsed)); - lastNominatimCall = Date.now(); -} - export async function reverseGeocodeCountry(lat: number, lng: number): Promise { - const key = roundKey(lat, lng); - if (geocodeCache.has(key)) return geocodeCache.get(key)!; - await throttleNominatim(); + const key = cacheKeyFor(lat, lng, 'country'); + const hit = getCached(key); + if (hit !== undefined) return hit; try { - const res = await fetch( - `https://nominatim.openstreetmap.org/reverse?lat=${lat}&lon=${lng}&format=json&zoom=3&accept-language=en`, - { - headers: { 'User-Agent': 'TREK Travel Planner (https://github.com/liketrek/TREK)' }, - signal: AbortSignal.timeout(10_000), - }, + const res = await nominatimFetch( + 'reverse', + new URLSearchParams({ lat: String(lat), lon: String(lng), format: 'json', zoom: '3', 'accept-language': 'en' }), + { lane: 'background', timeoutMs: 10_000 }, ); if (!res.ok) return null; const data = (await res.json()) as { address?: { country_code?: string } }; const code = data.address?.country_code?.toUpperCase() || null; - geocodeCache.set(key, code); + setCached(key, code); return code; } catch { return null; @@ -924,14 +878,11 @@ export async function getRegionFromCoords(countryCode: string, lat: number, lng: // Returns the OSM address object, {} for an "ok but empty" response (so it is cached as // a definitive miss), or null for a transient failure (so it is retried next time). async function fetchNominatimAddress(lat: number, lng: number, zoom: number): Promise | null> { - await throttleNominatim(); try { - const res = await fetch( - `https://nominatim.openstreetmap.org/reverse?lat=${lat}&lon=${lng}&format=json&zoom=${zoom}&accept-language=en`, - { - headers: { 'User-Agent': 'TREK Travel Planner (https://github.com/liketrek/TREK)' }, - signal: AbortSignal.timeout(10_000), - }, + const res = await nominatimFetch( + 'reverse', + new URLSearchParams({ lat: String(lat), lon: String(lng), format: 'json', zoom: String(zoom), 'accept-language': 'en' }), + { lane: 'background', timeoutMs: 10_000 }, ); if (!res.ok) return null; const data = (await res.json()) as { address?: Record }; @@ -975,7 +926,7 @@ function buildRegionInfo(address: Record, preferFinest: boolean) } export async function reverseGeocodeRegion(lat: number, lng: number, placeAddress?: string | null): Promise { - const key = roundKey(lat, lng); + const key = cacheKeyFor(lat, lng, 'region'); if (regionCache.has(key)) return regionCache.get(key)!; // Prefer resolving directly against the bundled polygons: offline, deterministic, and — diff --git a/server/src/nest/common/geo.ts b/server/src/nest/common/geo.ts index a6348d57f..20a4d2247 100644 --- a/server/src/nest/common/geo.ts +++ b/server/src/nest/common/geo.ts @@ -9,3 +9,21 @@ export function haversineKm(lat1: number, lng1: number, lat2: number, lng2: numb Math.cos(toRad(lat1)) * Math.cos(toRad(lat2)) * Math.sin(dLng / 2) ** 2; return R * 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a)); } + +/** + * The same distance in metres. + * + * There were three implementations of this in the tree: this one, a copy in + * maps.helpers.ts and a private one in places.helpers.ts that differed by a + * single clamp. The clamped form is the one kept — `asin` of a value a hair over + * 1, which floating point does produce for near-antipodal points, is NaN, and a + * NaN distance silently fails every comparison it feeds rather than throwing. + */ +export function haversineMetres(aLat: number, aLng: number, bLat: number, bLng: number): number { + const toRad = (deg: number) => (deg * Math.PI) / 180; + const dLat = toRad(bLat - aLat); + const dLng = toRad(bLng - aLng); + const h = + Math.sin(dLat / 2) ** 2 + Math.cos(toRad(aLat)) * Math.cos(toRad(bLat)) * Math.sin(dLng / 2) ** 2; + return 2 * 6371000 * Math.asin(Math.min(1, Math.sqrt(h))); +} diff --git a/server/src/nest/geo/geo.module.ts b/server/src/nest/geo/geo.module.ts new file mode 100644 index 000000000..2d8162da8 --- /dev/null +++ b/server/src/nest/geo/geo.module.ts @@ -0,0 +1,19 @@ +import { Module } from '@nestjs/common'; +import { GeocodingService } from './geocoding.service'; + +/** + * The single Nominatim seam. + * + * A leaf that imports nothing and is registered straight in AppModule, rather + * than imported by MapsModule and AtlasModule. That is not an oversight: both + * domains reach `nominatimFetch` as a module-level primitive, because the + * bridges have to reach the very same throttle cursor from outside the + * container. An import here would claim an injection edge that does not exist. + * What the module contributes is the lifecycle hook that starts and stops the + * cache's cleanup timer. + */ +@Module({ + providers: [GeocodingService], + exports: [GeocodingService], +}) +export class GeoModule {} diff --git a/server/src/nest/geo/geocoding.service.ts b/server/src/nest/geo/geocoding.service.ts new file mode 100644 index 000000000..7edd4dcb1 --- /dev/null +++ b/server/src/nest/geo/geocoding.service.ts @@ -0,0 +1,27 @@ +import { Injectable, OnModuleInit, OnModuleDestroy } from '@nestjs/common'; +import { startGeoCacheCleanup, stopGeoCacheCleanup } from './nominatim.client'; + +/** + * Owns the geo cache's cleanup timer, and nothing else. + * + * Deliberately not a facade over `nominatimFetch`. The throttle cursor and the + * cache have to be module state — the bridges construct their collaborators by + * hand, outside the container, and a second copy of the cursor is exactly the + * defect this domain exists to remove. A provider wrapping them would suggest + * per-instance state that is not there. + * + * What the container does add is a lifecycle. Atlas used to start this interval + * as an import-time side effect, documented as a parity exception; a timer + * nobody stops is how a test process ends up hanging, and the container has a + * hook for exactly this. + */ +@Injectable() +export class GeocodingService implements OnModuleInit, OnModuleDestroy { + onModuleInit(): void { + startGeoCacheCleanup(); + } + + onModuleDestroy(): void { + stopGeoCacheCleanup(); + } +} diff --git a/server/src/nest/geo/nominatim.client.ts b/server/src/nest/geo/nominatim.client.ts new file mode 100644 index 000000000..ec1562f94 --- /dev/null +++ b/server/src/nest/geo/nominatim.client.ts @@ -0,0 +1,166 @@ +import { UA } from '../maps/maps.helpers'; + +/** + * The one Nominatim client. + * + * There used to be two, and that is issue #576 — but not in the shape the issue + * describes. It was never two throttles racing: atlas throttled and cached, maps + * did neither. Five interactive maps routes fired straight at the service while + * two atlas background loops politely waited 1.1s between calls, and Nominatim + * saw one instance ignoring its rate limit. + * + * The state here is MODULE state, not provider state, and that is load-bearing + * rather than a style choice. A second instance of the cursor is the bug: the + * bridge instances and the DI singleton have to share one, exactly like the + * permissions and FX caches. `GeocodingService` next door owns the cleanup + * timer's lifecycle and nothing else. + * + * Every request carries `UA`, the instance-identifying User-Agent. Atlas used to + * send a bare generic string, which is the thing #1309 established gets throttled + * harder — so folding the two clients together had to adopt the stricter caller's + * header, not the weaker one. + */ + +const BASE = 'https://nominatim.openstreetmap.org'; +const MIN_INTERVAL_MS = 1100; + +let lastCall = 0; +let minIntervalMs = MIN_INTERVAL_MS; + +/** + * Test seam: the interval, in milliseconds. + * + * Nineteen maps cases stub fetch and drive the Nominatim paths back to back with + * real timers. Under the real interval each of them sleeps 1.1s, which buys the + * suite nineteen seconds of waiting and no coverage at all. `tests/setup.ts` + * sets it to 0; GEO-002 sets it back for the cases that are about the throttle. + * Deliberately a function rather than a NODE_ENV check — reading the environment + * inside a Nest module is what app-config exists to prevent, and a seam that is + * visible in the signature beats one that fires on an ambient variable. + */ +export function setGeoThrottleInterval(ms: number): void { + minIntervalMs = ms; +} + +/** + * Two lanes over one cursor. + * + * Interactive calls are on the request path of /api/maps/search, /autocomplete, + * /details, /reverse and /resolve-url. Background calls come from the atlas + * enrichment loops, which walk every uncached place. With a single queue a + * keystroke waits behind however many places the loop has enqueued, so + * interactive work takes the next slot and background work yields to it. + */ +export type GeoLane = 'interactive' | 'background'; + +let interactiveWaiting = 0; + +async function throttle(lane: GeoLane): Promise { + if (lane === 'interactive') interactiveWaiting++; + try { + for (;;) { + const elapsed = Date.now() - lastCall; + const wait = minIntervalMs - elapsed; + if (wait <= 0) break; + await new Promise((r) => setTimeout(r, wait)); + // A background call that woke up while an interactive one is queued gives + // up its slot rather than taking it. + if (lane === 'background' && interactiveWaiting > 0) { + await new Promise((r) => setTimeout(r, minIntervalMs)); + } + } + lastCall = Date.now(); + } finally { + if (lane === 'interactive') interactiveWaiting--; + } +} + +// ── Reverse-geocode cache ──────────────────────────────────────────────────── + +/** + * Keyed by coordinates AND the query shape. Atlas asks at zoom 3 (country) and + * zoom 8 (region), maps at zoom 18 (street): a key that carried coordinates alone + * would answer a street lookup with a country. It rounds to three decimals, a + * ~110m grid, which is the granularity the atlas cache always used. + */ +const cache = new Map(); +const CACHE_MAX = 50_000; +const CACHE_CLEANUP_MS = 10 * 60 * 1000; + +function roundKey(lat: number, lng: number): string { + return `${lat.toFixed(3)},${lng.toFixed(3)}`; +} + +export function cacheKeyFor(lat: number, lng: number, shape: string): string { + return `${shape}|${roundKey(lat, lng)}`; +} + +export function getCached(key: string): T | undefined { + return cache.has(key) ? (cache.get(key) as T) : undefined; +} + +export function setCached(key: string, value: unknown): void { + cache.set(key, value); +} + +/** Test seam: the suites need a known-empty cache between cases. */ +export function clearGeoCache(): void { + cache.clear(); + lastCall = 0; +} + +let cleanupTimer: ReturnType | null = null; + +/** + * Started by `GeocodingService.onModuleInit`, not at import time. + * + * The atlas module used to run this as an import-time side effect, documented as + * a deliberate parity exception. It does not have to be one any more: the + * container has a lifecycle hook for exactly this, and a timer nobody stops is + * how a test process ends up hanging. + */ +export function startGeoCacheCleanup(): void { + if (cleanupTimer) return; + cleanupTimer = setInterval(() => { + if (cache.size > CACHE_MAX) { + const keys = [...cache.keys()]; + for (const k of keys.slice(0, keys.length - CACHE_MAX)) cache.delete(k); + } + }, CACHE_CLEANUP_MS); + cleanupTimer.unref?.(); +} + +export function stopGeoCacheCleanup(): void { + if (!cleanupTimer) return; + clearInterval(cleanupTimer); + cleanupTimer = null; +} + +// ── The one outbound primitive ─────────────────────────────────────────────── + +export interface NominatimOptions { + lane?: GeoLane; + /** Caller's own deadline. Built AFTER the throttle wait, never before. */ + timeoutMs?: number; + signal?: AbortSignal; +} + +/** + * One request to Nominatim, throttled and identified. + * + * The abort signal is constructed after the throttle wait on purpose: building it + * first would spend up to 1.1s of the caller's budget queueing rather than + * fetching, which for the 2.5s identity lookup is nearly half of it. + */ +export async function nominatimFetch( + path: 'search' | 'reverse' | 'lookup', + params: URLSearchParams, + opts: NominatimOptions = {}, +): Promise { + await throttle(opts.lane ?? 'interactive'); + const signal = opts.signal ?? (opts.timeoutMs ? AbortSignal.timeout(opts.timeoutMs) : undefined); + return fetch(`${BASE}/${path}?${params.toString()}`, { + headers: { 'User-Agent': UA }, + ...(signal ? { signal } : {}), + }); +} diff --git a/server/src/nest/maps/maps.helpers.ts b/server/src/nest/maps/maps.helpers.ts index 3b15c0116..69d4b25fe 100644 --- a/server/src/nest/maps/maps.helpers.ts +++ b/server/src/nest/maps/maps.helpers.ts @@ -61,15 +61,9 @@ export function toWikiLang(lang: string | undefined, fallback = 'en'): string { return /^[a-z]{2,3}$/.test(base) ? base : fallback; } -/** Metres between two coordinates. Spherical earth is plenty at this scale. */ -export function haversineMetres(aLat: number, aLng: number, bLat: number, bLng: number): number { - const toRad = (deg: number) => (deg * Math.PI) / 180; - const dLat = toRad(bLat - aLat); - const dLng = toRad(bLng - aLng); - const h = - Math.sin(dLat / 2) ** 2 + Math.cos(toRad(aLat)) * Math.cos(toRad(bLat)) * Math.sin(dLng / 2) ** 2; - return 2 * 6371000 * Math.asin(Math.min(1, Math.sqrt(h))); -} +// Re-exported, not redefined: this was one of three copies. Kept as an export +// here because callers and a test import it from maps.helpers. +export { haversineMetres } from '../common/geo'; /** * Whether two place names plausibly refer to the same thing. diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 2b24a19a6..cb04e558f 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -13,6 +13,7 @@ import { decrypt_api_key } from '../common/crypto/apiKeyCrypto'; // ── Photo cache (disk-backed) ──────────────────────────────────────────────── import { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service'; import { DatabaseService } from '../database/database.service'; +import { nominatimFetch } from '../geo/nominatim.client'; import { UA, toApiLang, @@ -552,9 +553,9 @@ export class MapsService { limit: '10', 'accept-language': toApiLang(lang), }); - const response = await fetch(`https://nominatim.openstreetmap.org/search?${params}`, { - headers: { 'User-Agent': UA }, - }); + // Through the shared client: one throttle for the whole process, and the + // interactive lane so a keystroke does not queue behind the atlas backfill. + const response = await nominatimFetch('search', params); if (!response.ok) { const text = await response.text().catch(() => ''); throw new Error( @@ -626,9 +627,11 @@ export class MapsService { }); try { - const res = await fetch(`https://nominatim.openstreetmap.org/search?${params}`, { - headers: { 'User-Agent': UA }, - signal: opts.signal ?? AbortSignal.timeout(IDENTITY_TIMEOUT_MS), + // The caller's deadline is handed in rather than built here, so the + // throttle wait cannot eat it before the request starts. + const res = await nominatimFetch('search', params, { + signal: opts.signal, + timeoutMs: IDENTITY_TIMEOUT_MS, }); if (!res.ok) return null; // Nominatim answers rate limiting in plain text, not JSON. @@ -692,9 +695,7 @@ export class MapsService { 'accept-language': toApiLang(lang), }); try { - const res = await fetch(`https://nominatim.openstreetmap.org/lookup?${params}`, { - headers: { 'User-Agent': UA }, - }); + const res = await nominatimFetch('lookup', params); if (!res.ok) return null; const data = (await res.json()) as NominatimResult[]; const item = data[0]; @@ -1899,9 +1900,7 @@ export class MapsService { zoom: '18', 'accept-language': toApiLang(lang), }); - const response = await fetch(`https://nominatim.openstreetmap.org/reverse?${params}`, { - headers: { 'User-Agent': UA }, - }); + const response = await nominatimFetch('reverse', params); if (!response.ok) return { name: null, address: null }; const data = (await response.json()) as { name?: string; display_name?: string; address?: Record }; const addr = data.address || {}; @@ -1987,9 +1986,10 @@ export class MapsService { // Reverse geocode to get address. A non-ok answer (Nominatim 5xx/429) must // not fail the whole resolution — the coordinates are already extracted, so // fall back to the URL-derived name and a null address. - const nominatimRes = await fetch( - `https://nominatim.openstreetmap.org/reverse?lat=${lat}&lon=${lng}&format=json&addressdetails=1`, - { headers: { 'User-Agent': UA }, signal: AbortSignal.timeout(8000) }, + const nominatimRes = await nominatimFetch( + 'reverse', + new URLSearchParams({ lat: String(lat), lon: String(lng), format: 'json', addressdetails: '1' }), + { timeoutMs: 8000 }, ); const nominatim: { display_name?: string; name?: string; address?: Record } = nominatimRes.ok ? await nominatimRes.json() diff --git a/server/src/nest/places/places.helpers.ts b/server/src/nest/places/places.helpers.ts index 2f33fe3c8..2e6c2b702 100644 --- a/server/src/nest/places/places.helpers.ts +++ b/server/src/nest/places/places.helpers.ts @@ -4,6 +4,7 @@ import type { Place } from '../../types'; import type { PlaceWithTags } from '../database/database.service'; import type { KmlImportSummary } from './kml-import.helpers'; import type { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service'; +import { haversineMetres } from '../common/geo'; /** * Pure helpers and module-scope constants of the places domain, moved verbatim @@ -214,16 +215,6 @@ export const SEARCH_BIAS_RADIUS_METERS = 2000; /** Concurrent enrichment lookups — small, to stay friendly to the Maps quota. */ export const ENRICH_CONCURRENCY = 3; -function haversineMeters(a: { lat: number; lng: number }, b: { lat: number; lng: number }): number { - const R = 6371000; - const toRad = (d: number) => (d * Math.PI) / 180; - const dLat = toRad(b.lat - a.lat); - const dLng = toRad(b.lng - a.lng); - const lat1 = toRad(a.lat); - const lat2 = toRad(b.lat); - const h = Math.sin(dLat / 2) ** 2 + Math.cos(lat1) * Math.cos(lat2) * Math.sin(dLng / 2) ** 2; - return 2 * R * Math.asin(Math.sqrt(h)); -} /** * Pick the search result that is the same place as the import: it must be a @@ -245,7 +236,7 @@ export function pickEnrichmentMatch( const lng = c.lng; if (typeof gpid !== 'string' || !gpid) continue; if (typeof lat !== 'number' || typeof lng !== 'number') continue; - const dist = haversineMeters(target, { lat, lng }); + const dist = haversineMetres(target.lat, target.lng, lat, lng); if (dist > maxMeters) continue; if (!best || dist < best.dist) best = { c, dist }; } diff --git a/server/tests/setup.ts b/server/tests/setup.ts index e07dc1212..1451d303b 100644 --- a/server/tests/setup.ts +++ b/server/tests/setup.ts @@ -23,3 +23,15 @@ const realSetImmediate = globalThis.setImmediate; afterEach(async () => { await new Promise((resolve) => realSetImmediate(resolve)); }); + +// Nominatim's rate limit is a property of the real service, not of the code +// under test. Nineteen maps cases stub fetch and drive those paths back to back; +// under the real 1.1s interval they would spend nineteen seconds asleep. The +// cases that are actually about the throttle set it back for themselves. +// +// Imported dynamically, and down here rather than at the top: the client pulls +// in maps.helpers, whose UA constant calls getAppUrl() at module load. A static +// import is hoisted above the assignments above it, so the URL would be read +// from an environment this file has not finished setting up yet. +const { setGeoThrottleInterval } = await import('../src/nest/geo/nominatim.client'); +setGeoThrottleInterval(0); diff --git a/server/tests/unit/nest/atlas.service.test.ts b/server/tests/unit/nest/atlas.service.test.ts index a54bbc857..ddace305e 100644 --- a/server/tests/unit/nest/atlas.service.test.ts +++ b/server/tests/unit/nest/atlas.service.test.ts @@ -39,7 +39,8 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createTrip, createReservation } from '../../helpers/factories'; -import { getCached, setCache, getCountryFromCoords, getCountryFromAddress, isPointInCountryBox, reverseGeocodeCountry, getRegionGeo, getCountryGeo } from '../../../src/nest/atlas/atlas-geo'; +import { getCountryFromCoords, getCountryFromAddress, isPointInCountryBox, reverseGeocodeCountry, getRegionGeo, getCountryGeo } from '../../../src/nest/atlas/atlas-geo'; +import { cacheKeyFor, getCached, setCached } from '../../../src/nest/geo/nominatim.client'; import { DatabaseService } from '../../../src/nest/database/database.service'; import { AtlasService } from '../../../src/nest/atlas/atlas.service'; @@ -201,32 +202,43 @@ describe('getStats', () => { }); }); -// ── getCached / setCache ──────────────────────────────────────────────────── +// ── the shared geocode cache ────────────────────────────────────── -describe('getCached and setCache', () => { - it('ATLAS-SVC-001: getCached returns undefined for unknown coordinates', () => { - // Use uniquely large lat values to guarantee no prior cache entry - const result = getCached(9001.001, 9001.001); - expect(result).toBeUndefined(); +/** + * The cache moved to geo/ with the client, and its key gained the query shape. + * That is not cosmetic: atlas asks at zoom 3 and 8, maps at zoom 18, and a key + * of coordinates alone would answer a street lookup with a country name. + */ +const countryKey = (lat: number, lng: number) => cacheKeyFor(lat, lng, 'country'); + +describe('the shared geocode cache', () => { + it('ATLAS-SVC-001: a miss is undefined, not null', () => { + // Uniquely large lat values guarantee no prior entry. + expect(getCached(countryKey(9001.001, 9001.001))).toBeUndefined(); }); - it('ATLAS-SVC-002: setCache then getCached returns the stored code', () => { - setCache(9002.002, 9002.002, 'DE'); - const result = getCached(9002.002, 9002.002); - expect(result).toBe('DE'); + it('ATLAS-SVC-002: what goes in comes back out', () => { + setCached(countryKey(9002.002, 9002.002), 'DE'); + expect(getCached(countryKey(9002.002, 9002.002))).toBe('DE'); }); - it('ATLAS-SVC-003: setCache can store null (country unknown)', () => { - setCache(9003.003, 9003.003, null); - const result = getCached(9003.003, 9003.003); - expect(result).toBeNull(); + it('ATLAS-SVC-003: null is a stored value, meaning "asked, nobody knows"', () => { + setCached(countryKey(9003.003, 9003.003), null); + expect(getCached(countryKey(9003.003, 9003.003))).toBeNull(); }); - it('ATLAS-SVC-004: different coordinates return different cached values', () => { - setCache(9004.004, 9004.004, 'FR'); - setCache(9004.005, 9004.005, 'ES'); - expect(getCached(9004.004, 9004.004)).toBe('FR'); - expect(getCached(9004.005, 9004.005)).toBe('ES'); + it('ATLAS-SVC-004: different coordinates stay separate', () => { + setCached(countryKey(9004.004, 9004.004), 'FR'); + setCached(countryKey(9004.005, 9004.005), 'ES'); + expect(getCached(countryKey(9004.004, 9004.004))).toBe('FR'); + expect(getCached(countryKey(9004.005, 9004.005))).toBe('ES'); + }); + + it('ATLAS-SVC-004b: the same coordinates under different query shapes do not collide', () => { + setCached(cacheKeyFor(9005.005, 9005.005, 'country'), 'IT'); + setCached(cacheKeyFor(9005.005, 9005.005, 'region'), 'Tuscany'); + expect(getCached(cacheKeyFor(9005.005, 9005.005, 'country'))).toBe('IT'); + expect(getCached(cacheKeyFor(9005.005, 9005.005, 'region'))).toBe('Tuscany'); }); }); diff --git a/server/tests/unit/nest/geo.nominatim.test.ts b/server/tests/unit/nest/geo.nominatim.test.ts new file mode 100644 index 000000000..c4dcb61f9 --- /dev/null +++ b/server/tests/unit/nest/geo.nominatim.test.ts @@ -0,0 +1,201 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +/** + * The one Nominatim client (#576). + * + * The interesting behaviour is not the fetch — it is the state around it. There + * used to be two clients: atlas throttled and cached, maps did neither, and + * Nominatim saw a single instance ignoring its published rate limit. These cases + * pin the three properties that made folding them together worth doing: one + * cursor for the whole process, a cache key that cannot answer a street lookup + * with a country, and a deadline that starts when the request does rather than + * when it is queued. + */ +import { + cacheKeyFor, + clearGeoCache, + getCached, + nominatimFetch, + setCached, + setGeoThrottleInterval, + startGeoCacheCleanup, + stopGeoCacheCleanup, +} from '../../../src/nest/geo/nominatim.client'; +import { GeocodingService } from '../../../src/nest/geo/geocoding.service'; +import { UA } from '../../../src/nest/maps/maps.helpers'; + +const fetchMock = vi.fn(); + +beforeEach(() => { + vi.useFakeTimers(); + // tests/setup.ts zeroes this for every other suite. This one is about the + // throttle, so it runs against the interval the real service publishes. + setGeoThrottleInterval(1100); + clearGeoCache(); + fetchMock.mockReset(); + fetchMock.mockResolvedValue({ ok: true, json: async () => ({}) }); + vi.stubGlobal('fetch', fetchMock); +}); + +afterEach(() => { + stopGeoCacheCleanup(); + setGeoThrottleInterval(0); + vi.useRealTimers(); + vi.unstubAllGlobals(); +}); + +/** Runs `fn`, draining fake timers until it settles. */ +async function drain(fn: () => Promise): Promise { + const p = fn(); + await vi.runAllTimersAsync(); + return p; +} + +describe('nominatimFetch', () => { + it('GEO-001: identifies the instance on every request', async () => { + await drain(() => nominatimFetch('search', new URLSearchParams({ q: 'Berlin' }))); + + const [url, init] = fetchMock.mock.calls[0]; + expect(url).toBe('https://nominatim.openstreetmap.org/search?q=Berlin'); + // Atlas used to send a bare generic string. Folding the two clients together + // had to adopt the stricter caller's header, not the weaker one — #1309 + // established that a generic UA gets throttled harder. + expect(init.headers['User-Agent']).toBe(UA); + }); + + it('GEO-002: holds a shared cursor, so a second caller waits out the rate limit', async () => { + await drain(() => nominatimFetch('search', new URLSearchParams({ q: 'a' }))); + expect(fetchMock).toHaveBeenCalledTimes(1); + + const second = nominatimFetch('search', new URLSearchParams({ q: 'b' })); + await vi.advanceTimersByTimeAsync(500); + // Still inside the 1.1s window — this is the whole point of one client. + expect(fetchMock).toHaveBeenCalledTimes(1); + + await vi.advanceTimersByTimeAsync(700); + await second; + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('GEO-003: builds the deadline after the throttle wait, not before it', async () => { + // Asserted on WHEN the signal is constructed rather than on when it fires: + // AbortSignal.timeout runs on libuv, not on the mocked global setTimeout, so + // advancing the fake clock would never abort it and the assertion would hold + // whether or not the deadline had already been half spent. + const madeAt: number[] = []; + const real = AbortSignal.timeout.bind(AbortSignal); + vi.spyOn(AbortSignal, 'timeout').mockImplementation((ms: number) => { + madeAt.push(Date.now()); + return real(ms); + }); + + await drain(() => nominatimFetch('search', new URLSearchParams({ q: 'a' }))); + const queuedAt = Date.now(); + + // A queued call with a 2.5s budget: built up front, the 1.1s spent waiting + // for the rate limit would eat nearly half of it before the socket opens. + const queued = nominatimFetch('search', new URLSearchParams({ q: 'b' }), { timeoutMs: 2500 }); + await vi.runAllTimersAsync(); + await queued; + + expect(madeAt).toHaveLength(1); + expect(madeAt[0] - queuedAt).toBeGreaterThanOrEqual(1100); + expect(fetchMock.mock.calls[1][1].signal).toBeInstanceOf(AbortSignal); + }); + + it("GEO-004: a caller's own signal wins over the timeout", async () => { + const controller = new AbortController(); + await drain(() => + nominatimFetch('reverse', new URLSearchParams({ lat: '1' }), { + signal: controller.signal, + timeoutMs: 100, + }), + ); + expect(fetchMock.mock.calls[0][1].signal).toBe(controller.signal); + }); + + it('GEO-005: sends no signal at all when the caller asked for no deadline', async () => { + await drain(() => nominatimFetch('lookup', new URLSearchParams({ osm_ids: 'N1' }))); + expect(fetchMock.mock.calls[0][1].signal).toBeUndefined(); + }); + + it('GEO-006: a background call yields its slot to an interactive one', async () => { + await drain(() => nominatimFetch('search', new URLSearchParams({ q: 'seed' }))); + + const background = nominatimFetch('search', new URLSearchParams({ q: 'bg' }), { lane: 'background' }); + const interactive = nominatimFetch('search', new URLSearchParams({ q: 'ui' })); + + await vi.runAllTimersAsync(); + await Promise.all([background, interactive]); + + // The atlas backfill walks every uncached place; a keystroke must not queue + // behind it. + const order = fetchMock.mock.calls.slice(1).map(([url]) => (url as string).includes('q=ui') ? 'ui' : 'bg'); + expect(order[0]).toBe('ui'); + }); +}); + +describe('geo cache', () => { + it('GEO-007: keys on the query shape, not coordinates alone', async () => { + // Atlas asks at zoom 3 (country) and zoom 8 (region), maps at zoom 18 + // (street). A coordinate-only key would answer a street lookup with a country. + const street = cacheKeyFor(52.52, 13.405, 'zoom18'); + const country = cacheKeyFor(52.52, 13.405, 'zoom3'); + expect(street).not.toBe(country); + + setCached(country, { country: 'DE' }); + expect(getCached(street)).toBeUndefined(); + expect(getCached(country)).toEqual({ country: 'DE' }); + }); + + it('GEO-008: rounds to a ~110m grid, so a nudged coordinate still hits', () => { + expect(cacheKeyFor(52.52001, 13.40501, 'zoom18')).toBe(cacheKeyFor(52.5200, 13.4050, 'zoom18')); + expect(cacheKeyFor(52.53, 13.405, 'zoom18')).not.toBe(cacheKeyFor(52.52, 13.405, 'zoom18')); + }); + + it('GEO-009: distinguishes a cached undefined from a miss', () => { + const key = cacheKeyFor(0, 0, 'zoom3'); + expect(getCached(key)).toBeUndefined(); + setCached(key, undefined); + // A negative answer is worth caching: the ocean does not become a country. + // `has` rather than a truthy check is what keeps it from being re-fetched. + expect(getCached(key)).toBeUndefined(); + }); + + it('GEO-010: the cleanup timer is idempotent and stoppable', () => { + const spy = vi.spyOn(global, 'setInterval'); + startGeoCacheCleanup(); + startGeoCacheCleanup(); + // Started twice by two modules booting would otherwise leak an interval. + expect(spy).toHaveBeenCalledTimes(1); + + stopGeoCacheCleanup(); + startGeoCacheCleanup(); + expect(spy).toHaveBeenCalledTimes(2); + spy.mockRestore(); + }); + + it('GEO-011b: the container owns the timer, so a shut-down app leaves none running', () => { + const spy = vi.spyOn(global, 'clearInterval'); + const svc = new GeocodingService(); + svc.onModuleInit(); + svc.onModuleDestroy(); + // Atlas used to start this at import time and never stop it — a timer nobody + // clears is how a test process ends up hanging past its last assertion. + expect(spy).toHaveBeenCalled(); + svc.onModuleDestroy(); + expect(spy).toHaveBeenCalledTimes(1); // idempotent, not a double clear + spy.mockRestore(); + }); + + it('GEO-011: trims the oldest entries once the cap is passed', () => { + startGeoCacheCleanup(); + for (let i = 0; i < 50_010; i++) setCached(`k${i}`, i); + expect(getCached('k0')).toBe(0); + + vi.advanceTimersByTime(10 * 60 * 1000); + // Insertion-ordered: the ten oldest go, the newest stay. + expect(getCached('k0')).toBeUndefined(); + expect(getCached('k50009')).toBe(50_009); + }); +}); diff --git a/server/vitest.config.ts b/server/vitest.config.ts index 7d64ee6e7..624d3e7bd 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -84,12 +84,13 @@ export default defineConfig({ 'src/nest/days/**/*.ts': { statements: 92, branches: 82, functions: 98, lines: 97 }, 'src/nest/feeds/**/*.ts': { statements: 91, branches: 83, functions: 83, lines: 91 }, 'src/nest/files/**/*.ts': { statements: 97, branches: 95, functions: 99, lines: 98 }, + 'src/nest/geo/**/*.ts': { statements: 99, branches: 95, functions: 99, lines: 99 }, 'src/nest/health/**/*.ts': { statements: 99, branches: 65, functions: 99, lines: 99 }, 'src/nest/help/**/*.ts': { statements: 81, branches: 70, functions: 99, lines: 86 }, - 'src/nest/integrations/**/*.ts': { statements: 69, branches: 52, functions: 72, lines: 71 }, + 'src/nest/integrations/**/*.ts': { statements: 72, branches: 56, functions: 76, lines: 74 }, 'src/nest/journey/**/*.ts': { statements: 90, branches: 84, functions: 83, lines: 91 }, 'src/nest/llm-parse/**/*.ts': { statements: 91, branches: 85, functions: 85, lines: 94 }, - 'src/nest/maps/**/*.ts': { statements: 92, branches: 85, functions: 95, lines: 95 }, + 'src/nest/maps/**/*.ts': { statements: 93, branches: 86, functions: 97, lines: 96 }, 'src/nest/memories/**/*.ts': { statements: 88, branches: 80, functions: 95, lines: 90 }, 'src/nest/notifications/**/*.ts': { statements: 83, branches: 72, functions: 84, lines: 86 }, 'src/nest/oauth/**/*.ts': { statements: 96, branches: 95, functions: 97, lines: 97 }, @@ -107,7 +108,7 @@ export default defineConfig({ 'src/nest/plugins/**/*.ts': { statements: 86, branches: 81, functions: 78, lines: 89 }, 'src/nest/query-helpers/**/*.ts': { statements: 90, branches: 75, functions: 99, lines: 92 }, 'src/nest/realtime/**/*.ts': { statements: 99, branches: 100, functions: 99, lines: 99 }, - 'src/nest/reservation-import/**/*.ts': { statements: 58, branches: 52, functions: 39, lines: 58 }, + 'src/nest/reservation-import/**/*.ts': { statements: 61, branches: 55, functions: 41, lines: 61 }, 'src/nest/reservations/**/*.ts': { statements: 92, branches: 83, functions: 93, lines: 95 }, 'src/nest/settings/**/*.ts': { statements: 85, branches: 68, functions: 99, lines: 87 }, 'src/nest/share/**/*.ts': { statements: 97, branches: 87, functions: 99, lines: 99 },