One Nominatim client for atlas and maps

Atlas throttled to the 1.1s OSM asks for and cached its answers in a 50k
map; maps did neither, so five interactive routes fired straight at the
service while the two atlas backfill loops politely waited their turn.
From Nominatim's side that is one instance ignoring the rate limit.

Both go through geo/nominatim.client.ts now. Three things the naive merge
would have got wrong:

- The shared client sends UA, the instance-identifying User-Agent, not the
  generic string atlas used. A shared generic UA is throttled harder.
- One cursor across five interactive routes and two background loops would
  have put a keystroke behind however many places the backfill enqueued, so
  there are two lanes over the one cursor and background yields.
- The cache key carries the query shape. Atlas asks at zoom 3 and 8, maps at
  18; a coordinate-only key answers a street lookup with a country.

The throttle cursor and the cache stay module state, the way the permissions
and FX caches do, because the bridges build their collaborators outside the
container and a second copy of the cursor is the bug being removed here.
GeocodingService owns only the cleanup timer's lifecycle, which atlas used to
start as an import-time side effect.

Also folds the three haversine implementations into common/geo.ts, keeping
the clamped form: asin of a value a hair over 1 is NaN, and a NaN distance
fails every comparison it feeds instead of throwing.

tests/setup.ts zeroes the interval. Nineteen maps cases drive these paths
back to back with real timers and were sleeping nineteen seconds for it.
This commit is contained in:
Maurice
2026-08-10 15:25:09 +02:00
committed by Maurice
parent 55da861de6
commit 328b5fcb2b
14 changed files with 552 additions and 125 deletions
+37 -3
View File
@@ -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
+2 -1
View File
@@ -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
+14 -63
View File
@@ -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<Map<string, string>> {
});
}
// ── Geocode cache ───────────────────────────────────────────────────────────
const geocodeCache = new Map<string, string | null>();
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<string, string> = {
// ── 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<string | null> {
const key = roundKey(lat, lng);
if (geocodeCache.has(key)) return geocodeCache.get(key)!;
await throttleNominatim();
const key = cacheKeyFor(lat, lng, 'country');
const hit = getCached<string | null>(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<Record<string, string> | 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<string, string> };
@@ -975,7 +926,7 @@ function buildRegionInfo(address: Record<string, string>, preferFinest: boolean)
}
export async function reverseGeocodeRegion(lat: number, lng: number, placeAddress?: string | null): Promise<RegionInfo | null> {
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 —
+18
View File
@@ -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)));
}
+19
View File
@@ -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 {}
+27
View File
@@ -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();
}
}
+166
View File
@@ -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<void> {
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<string, unknown>();
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<T>(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<typeof setInterval> | 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<Response> {
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 } : {}),
});
}
+3 -9
View File
@@ -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.
+15 -15
View File
@@ -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<string, string> };
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<string, string> } = nominatimRes.ok
? await nominatimRes.json()
+2 -11
View File
@@ -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 };
}
+12
View File
@@ -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);
+32 -20
View File
@@ -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');
});
});
@@ -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<T>(fn: () => Promise<T>): Promise<T> {
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);
});
});
+4 -3
View File
@@ -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 },