diff --git a/client/src/sync/tripSyncManager.prepare.test.ts b/client/src/sync/tripSyncManager.prepare.test.ts index efe7fd030..7f9088369 100644 --- a/client/src/sync/tripSyncManager.prepare.test.ts +++ b/client/src/sync/tripSyncManager.prepare.test.ts @@ -9,6 +9,7 @@ import { tripSyncManager, type PrepareProgress } from './tripSyncManager' import { prefetchTilesForTrip } from './tilePrefetcher' import { setAuthed } from './authGate' import { setCacheTiles, setTripOfflineEnabled, _resetOfflinePrefs } from './offlinePrefs' +import { prefetchPlacesForTrip } from './placePrefetcher' import { offlineDb, clearAll, upsertTrip } from '../db/offlineDb' import { buildTrip, buildDay, buildPlace, buildTripFile } from '../../tests/helpers/factories' import type { Trip, TripFile } from '../types' @@ -16,8 +17,12 @@ import type { Trip, TripFile } from '../types' vi.mock('./tilePrefetcher', () => ({ prefetchTilesForTrip: vi.fn(async () => {}), })) +vi.mock('./placePrefetcher', () => ({ + prefetchPlacesForTrip: vi.fn(async () => 0), +})) const prefetchMock = vi.mocked(prefetchTilesForTrip) +const placesMock = vi.mocked(prefetchPlacesForTrip) function dateOffset(days: number): string { const d = new Date() @@ -62,6 +67,8 @@ beforeEach(async () => { setOnline(true) prefetchMock.mockClear() prefetchMock.mockResolvedValue(undefined) + placesMock.mockClear() + placesMock.mockResolvedValue(0) vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true, blob: async () => new Blob(['pdf-bytes'], { type: 'application/pdf' }), @@ -158,6 +165,13 @@ describe('tripSyncManager.prepareForOffline — full run', () => { expect(progress.map(p => p.phase)).toEqual(['trips', 'files', 'done']) expect(prefetchMock).not.toHaveBeenCalled() + // The places still come down. That switch is about tens of megabytes of + // imagery; a trip's places are about one, and hanging them off it meant + // somebody who turned it off for space got an empty offline search with no + // explanation, while this very run reported success. + expect(placesMock).toHaveBeenCalledTimes(1) + expect(placesMock.mock.calls[0][0]).toBe(604) + expect(placesMock.mock.calls[0][2]).toBe(true) }) it('FE-SYNC-PREP-008: caches the global tags and categories', async () => { diff --git a/client/src/sync/tripSyncManager.ts b/client/src/sync/tripSyncManager.ts index cc902656a..a5356900c 100644 --- a/client/src/sync/tripSyncManager.ts +++ b/client/src/sync/tripSyncManager.ts @@ -268,19 +268,25 @@ export const tripSyncManager = { // after login, where the app is still mounting the first screen — starting // a bulk tile download into that leaves the UI waiting behind our own // background traffic. - if (cacheTiles) { - whenIdle(async () => { - for (const trip of toSync) { - if (!isAuthed() || !navigator.onLine) return - const places = await offlineDb.places.where('trip_id').equals(trip.id).toArray() + // The place cache is not tied to the tile setting. That switch is about + // map tiles — "off keeps the cache to trip data + documents only" — and it + // is turned off for space, tens of megabytes of them. A trip's places are + // about a megabyte, and hanging them off that switch meant somebody who + // turned it off got an empty offline search with no explanation, while + // "prepare for offline" still reported success having stored no place at + // all. + whenIdle(async () => { + for (const trip of toSync) { + if (!isAuthed() || !navigator.onLine) return + const places = await offlineDb.places.where('trip_id').equals(trip.id).toArray() + // Tiles first when they are wanted: they are the bigger win, and the + // two should not compete for the connection. + if (cacheTiles) { await prefetchTilesForTrip(trip.id, places, tileUrl, undefined, cartoKey).catch(console.error) - // The places around the trip, so search answers with no network. - // Sequential rather than parallel: the tiles are the bigger win and - // should not be competing with this for the connection. - await prefetchPlacesForTrip(trip.id, places).catch(console.error) } - }) - } + await prefetchPlacesForTrip(trip.id, places).catch(console.error) + } + }) return { status: 'done', trips: toSync.length } } finally { _syncing = false @@ -332,16 +338,25 @@ export const tripSyncManager = { await cacheFilesForTrip(trip.id, files).catch(console.error) } - // 3) Map tiles — awaited, and only when the user opted to store them. - if (getOfflinePrefs().cacheTiles) { + // 3) Map tiles — awaited, and only when the user opted to store them. The + // places go with every run: "prepared" has to mean the offline search + // works, and the tile switch is about tens of megabytes of imagery, not + // about the megabyte a trip's places cost. + { + const wantTiles = getOfflinePrefs().cacheTiles const tileUrl = useSettingsStore.getState().settings.map_tile_url || undefined const cartoKey = useSettingsStore.getState().settings.carto_api_key || undefined i = 0 for (const trip of toSync) { if (!isAuthed()) return { status: 'skipped', reason: 'signed-out' } - onProgress?.({ phase: 'tiles', current: ++i, total, label: trip.title }) + // Reported only when there is a tile phase to report. The place + // prefetch rides along quietly, the way it always did — it is a + // megabyte, not the tens the progress bar exists for. + if (wantTiles) onProgress?.({ phase: 'tiles', current: ++i, total, label: trip.title }) const places = await offlineDb.places.where('trip_id').equals(trip.id).toArray() - await prefetchTilesForTrip(trip.id, places, tileUrl, true, cartoKey).catch(console.error) + if (wantTiles) { + await prefetchTilesForTrip(trip.id, places, tileUrl, true, cartoKey).catch(console.error) + } await prefetchPlacesForTrip(trip.id, places, true).catch(console.error) } } diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index 6273df6ec..e032b91dd 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -97,6 +97,51 @@ function instanceToken(): string { return `trek-${(hash >>> 0).toString(36)}`; } +/** + * Stop asking a service that is not answering. + * + * Every call site here is in front of something a person is waiting for, and + * each one falls back to the old path when the index fails — but it pays the + * timeout first, every single time. An instance behind a firewall that drops + * rather than refuses, or one whose network simply cannot reach the service, + * therefore pays it on every keystroke of autocomplete and on every one of the + * three queries a booking import runs per venue. Ten venues is over a hundred + * seconds of waiting for an answer that was never going to come. + * + * So: after a few consecutive failures, fail instantly for a while instead of + * dialling out. One request pays the timeout, the rest are free, and after the + * cooldown the next call tries again for real — if it works, the count resets + * and nothing was permanently switched off. + * + * Counted per process, deliberately. This guards the instance's own latency, + * not the service, so it neither needs nor deserves storage. + */ +const BREAKER_FAILURES_BEFORE_OPEN = 4; +const BREAKER_COOLDOWN_MS = 60_000; +let breakerFailures = 0; +let breakerOpenUntil = 0; + +/** + * Whether a failure says anything about reachability. + * + * A 404 does not: "no such place" is a perfectly good answer, and + * trekPlacesById turns it into null. Nor does an oversized body, which means + * the service answered and answered too much. What counts is a request that + * never came back, and a server error, because both mean asking again costs the + * same and returns the same. + */ +function countsAgainstReachability(err: unknown): boolean { + const status = (err as { status?: number } | null)?.status; + if (typeof status === 'number') return status >= 500 || status === 429; + return !(err instanceof Error && err.message === 'TREK Places API response too large'); +} + +/** Test seam: the breaker is process state, so a suite has to be able to clear it. */ +export function resetTrekPlacesBreaker(): void { + breakerFailures = 0; + breakerOpenUntil = 0; +} + export function trekPlacesBaseUrl(): string { const configured = (readEnv().maps.trekPlacesUrl || '').trim(); return (configured || DEFAULT_TREK_PLACES_URL).replace(/\/+$/, ''); @@ -112,6 +157,10 @@ async function getJson( if (v !== undefined && v !== null && v !== '') url.searchParams.set(k, String(v)); } + if (Date.now() < breakerOpenUntil) { + throw new Error('TREK Places API unreachable, not retrying yet'); + } + const controller = new AbortController(); const timer = setTimeout(() => controller.abort(), opts.timeoutMs ?? TIMEOUT_MS); try { @@ -130,7 +179,25 @@ async function getJson( // Read with a cap rather than res.json(): a hostile or broken upstream // should not be able to hand us an unbounded body to buffer. const text = await readCapped(res, opts.maxBytes ?? MAX_BYTES); - return JSON.parse(text) as T; + const parsed = JSON.parse(text) as T; + // An answer of any kind means the service is there. + breakerFailures = 0; + return parsed; + } catch (err: unknown) { + if (countsAgainstReachability(err)) { + breakerFailures += 1; + if (breakerFailures >= BREAKER_FAILURES_BEFORE_OPEN) { + breakerOpenUntil = Date.now() + BREAKER_COOLDOWN_MS; + // Back to the threshold rather than to zero: when the cooldown ends, + // one more failure re-opens the breaker instead of buying another four + // full timeouts. + breakerFailures = BREAKER_FAILURES_BEFORE_OPEN - 1; + } + } else { + // The service answered. A 404 is an answer. + breakerFailures = 0; + } + throw err; } finally { clearTimeout(timer); } diff --git a/server/tests/unit/nest/maps.search-merge.test.ts b/server/tests/unit/nest/maps.search-merge.test.ts new file mode 100644 index 000000000..24a5aa2c9 --- /dev/null +++ b/server/tests/unit/nest/maps.search-merge.test.ts @@ -0,0 +1,189 @@ +/** + * MAPS-SEARCH-001..008 — the two sources answering one explicit search. + * + * mergeSearchResults is tested on its own next door; this is the wiring around + * it, which had no test at all. What the wiring decides is not cosmetic: which + * list goes in first (the whole point of the feature is that the landmark lands + * at position two rather than eleven), and what `source` says — the client + * switches on it and the search log writes it into the corpus a future index + * gets scored against. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { mockSearch, mockNominatim } = vi.hoisted(() => ({ + mockSearch: vi.fn(async (_query: string, _opts?: Record): Promise => []), + mockNominatim: vi.fn( + async (_endpoint: string, _params: URLSearchParams, _opts?: { lane?: string }): Promise => ({ + ok: true, + json: async () => [], + }), + ), +})); +vi.mock('../../../src/nest/maps/trek-places.client', async (importOriginal) => ({ + ...(await importOriginal()), + trekPlacesSearch: mockSearch, +})); +vi.mock('../../../src/nest/geo/nominatim.client', async (importOriginal) => ({ + ...(await importOriginal()), + nominatimFetch: mockNominatim, +})); + +vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KEY: '0'.repeat(64) })); + +import { MapsService } from '../../../src/nest/maps/maps.service'; +import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; + +/** An index row, far enough from the OSM one below not to be deduped. */ +const indexHit = (name: string, lat = 35.31, lng = 139.53) => ({ + gers: name.toLowerCase().replace(/\W/g, ''), + name, + lat, + lng, + address: { freeform: null, locality: 'Kamakura', postcode: null, region: null, country: 'JP' }, + contact: { website: null, phone: null, email: null, socials: null }, + source: 'overture', + hours: null, +}); + +const osmRow = (name: string, lat = 35.32, lng = 139.55) => ({ + osm_type: 'way', + osm_id: 1234 + Math.round(lat * 1000), + lat: String(lat), + lon: String(lng), + name, + display_name: `${name}, Kamakura, Japan`, +}); + +const osmAnswer = (rows: unknown[]) => ({ ok: true, json: async () => rows }); + +/** + * Keyed on the statement so the kill switch can be driven without also handing + * `'false'` to the API-key resolver, which would send the fallback at Google. + */ +function make(enabled = true) { + const database = { + get: vi.fn((sql: string) => + sql.includes('trek_places_enabled') && !enabled ? { value: 'false' } : undefined, + ), + } as unknown as DatabaseService; + return new MapsService(database, {} as PlacePhotoCacheService); +} + +beforeEach(() => { + mockSearch.mockReset(); + mockSearch.mockResolvedValue([]); + mockNominatim.mockReset(); + mockNominatim.mockResolvedValue(osmAnswer([])); + vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +describe('MapsService.searchPlaces — the merged path', () => { + it('MAPS-SEARCH-001: both sources answer, index first, and says so', async () => { + mockSearch.mockResolvedValue([indexHit('Shop by the gate')]); + mockNominatim.mockResolvedValue(osmAnswer([osmRow('長谷寺')])); + + const out = await make().searchPlaces(1, 'Hase-dera'); + + expect(out.source).toBe('trek-places+openstreetmap'); + expect(out.places).toHaveLength(2); + // The whole point of the feature: the landmark OpenStreetMap knows lands at + // position two, not at position eleven behind ten shops. + expect(out.places[0].name).toBe('Shop by the gate'); + expect(out.places[1].name).toBe('長谷寺'); + }); + + it('MAPS-SEARCH-002: an index answer with nothing from OSM is named as index only', async () => { + mockSearch.mockResolvedValue([indexHit("L'Osteria")]); + mockNominatim.mockResolvedValue(osmAnswer([])); + + const out = await make().searchPlaces(1, "L'Osteria"); + + // `source` is not decoration: the search log writes it into the corpus a + // candidate index is scored against later, so a wrong label poisons the + // measurement rather than just the display. + expect(out.source).toBe('trek-places'); + expect(out.places).toHaveLength(1); + }); + + it('MAPS-SEARCH-003: OpenStreetMap failing costs its half, not the search', async () => { + mockSearch.mockResolvedValue([indexHit("L'Osteria")]); + mockNominatim.mockRejectedValue(new Error('nominatim down')); + + const out = await make().searchPlaces(1, "L'Osteria"); + + expect(out.source).toBe('trek-places'); + expect(out.places).toHaveLength(1); + expect(console.warn).toHaveBeenCalledWith('OpenStreetMap search failed, index only:', 'nominatim down'); + }); + + it('MAPS-SEARCH-004: the index failing keeps the answer OpenStreetMap already gave', async () => { + // The index turning a query down is ordinary traffic, not an outage: a + // common single word without coordinates is refused upstream as too + // expensive. Discarding the OSM results here and asking Nominatim again + // cost a second throttled round trip for a list that was already in hand. + mockSearch.mockRejectedValue(new Error('places api down')); + mockNominatim.mockResolvedValue(osmAnswer([osmRow('Museum')])); + + const out = await make().searchPlaces(1, 'museum'); + + expect(out.places).toHaveLength(1); + expect(out.places[0].name).toBe('Museum'); + expect(console.warn).toHaveBeenCalledWith('TREK Places search failed, falling back:', 'places api down'); + // One question, asked once. + expect(mockNominatim).toHaveBeenCalledTimes(1); + }); + + it('MAPS-SEARCH-005: both empty asks nobody a second time', async () => { + mockSearch.mockResolvedValue([]); + mockNominatim.mockResolvedValue(osmAnswer([])); + + const out = await make().searchPlaces(1, 'qwertzuiop'); + + expect(out.places).toEqual([]); + expect(out.source).toBe('openstreetmap'); + // The first call carried a viewbox with bounded=0, which orders results + // without changing which exist — so a second call can only return the same + // empty list a throttle interval later. + expect(mockNominatim).toHaveBeenCalledTimes(1); + }); + + it('MAPS-SEARCH-006: ten results at most, alternating so neither source is buried', async () => { + mockSearch.mockResolvedValue( + Array.from({ length: 12 }, (_, i) => indexHit(`Index ${i}`, 35.31 + i / 100)), + ); + mockNominatim.mockResolvedValue( + osmAnswer(Array.from({ length: 12 }, (_, i) => osmRow(`Osm ${i}`, 36.5 + i / 100))), + ); + + const out = await make().searchPlaces(1, 'anything'); + + expect(out.places).toHaveLength(10); + expect(out.places[0].name).toBe('Index 0'); + expect(out.places[1].name).toBe('Osm 0'); + expect(out.places[2].name).toBe('Index 1'); + }); + + it('MAPS-SEARCH-007: with the index switched off the query never leaves for it', async () => { + mockSearch.mockResolvedValue([indexHit("L'Osteria")]); + mockNominatim.mockResolvedValue(osmAnswer([osmRow('Steinstrasse')])); + + const out = await make(false).searchPlaces(1, "L'Osteria"); + + expect(mockSearch).not.toHaveBeenCalled(); + expect(out.source).toBe('openstreetmap'); + }); + + it('MAPS-SEARCH-008: the location bias reaches both sources', async () => { + mockSearch.mockResolvedValue([indexHit("L'Osteria")]); + + await make().searchPlaces(1, "L'Osteria", 'de', { lat: 54.0879, lng: 12.1408, radius: 5000 }); + + expect(mockSearch).toHaveBeenCalledWith("L'Osteria", { lat: 54.0879, lng: 12.1408, limit: 10 }); + // Without the viewbox, "Hase-dera" answers with the temple in Nara rather + // than the one the user is standing beside in Kamakura. + const params = mockNominatim.mock.calls[0][1] as URLSearchParams; + expect(params.get('viewbox')).toBeTruthy(); + expect(params.get('bounded')).toBe('0'); + }); +}); diff --git a/server/tests/unit/nest/trek-places.client.test.ts b/server/tests/unit/nest/trek-places.client.test.ts index 32a992d35..3d13432db 100644 --- a/server/tests/unit/nest/trek-places.client.test.ts +++ b/server/tests/unit/nest/trek-places.client.test.ts @@ -23,6 +23,7 @@ import { trekPlacesById, trekPlacesNearby, trekPlacesSearch, + resetTrekPlacesBreaker, type TrekPlace, } from '../../../src/nest/maps/trek-places.client'; @@ -87,6 +88,8 @@ function stubStreamingFetch(chunks: (string | Uint8Array)[], status = 200) { beforeEach(() => { calls = []; + // Process state, so it leaks between cases unless it is cleared. + resetTrekPlacesBreaker(); env.maps.trekPlacesUrl = ''; env.app.appUrl = 'https://trip.example.org'; // Nothing in this file may reach the real service: a test that forgets to @@ -241,6 +244,79 @@ describe('reading the body', () => { }); }); +describe('the circuit breaker', () => { + const BIG_BODY = new Uint8Array(600_000); + + // Every call site here sits in front of something a person is waiting for and + // falls back to the old path when the index fails — but it pays the timeout + // first, every time. On an instance that cannot reach the service at all, + // that is 3.5 s per keystroke of autocomplete, and over a hundred seconds for + // a ten-venue booking import. + + it('stops dialling out after a few consecutive failures', async () => { + stubFetch({}, 503); + for (let i = 0; i < 4; i++) { + await expect(trekPlacesSearch('x')).rejects.toThrow(); + } + const dialled = calls.length; + + // The next call must not reach the network at all. + await expect(trekPlacesSearch('x')).rejects.toThrow('unreachable, not retrying yet'); + expect(calls.length).toBe(dialled); + }); + + it('tries again once the cooldown is over', async () => { + vi.useFakeTimers(); + try { + stubFetch({}, 503); + for (let i = 0; i < 4; i++) await expect(trekPlacesSearch('x')).rejects.toThrow(); + const dialled = calls.length; + await expect(trekPlacesSearch('x')).rejects.toThrow('unreachable'); + expect(calls.length).toBe(dialled); + + vi.advanceTimersByTime(60_000); + stubFetch({ query: 'x', results: [PLACE] }); + await expect(trekPlacesSearch('x')).resolves.toHaveLength(1); + } finally { + vi.useRealTimers(); + } + }); + + it('an answer clears the count, so scattered failures never open it', async () => { + for (let i = 0; i < 20; i++) { + stubFetch({}, 503); + await expect(trekPlacesSearch('x')).rejects.toThrow(/503/); + stubFetch({ query: 'x', results: [] }); + await expect(trekPlacesSearch('x')).resolves.toEqual([]); + } + // Still dialling: the failures never ran consecutively. + stubFetch({}, 503); + await expect(trekPlacesSearch('x')).rejects.toThrow(/503/); + }); + + it('a 404 is an answer, not an outage', async () => { + // "No such place" says the service is there and working. Counting it would + // open the breaker for a caller who asked about places that do not exist. + for (let i = 0; i < 10; i++) { + stubFetch({}, 404); + await expect(trekPlacesById('nope')).resolves.toBeNull(); + } + stubFetch({ place: PLACE }); + await expect(trekPlacesById('abc-123')).resolves.toMatchObject({ gers: 'abc-123' }); + }); + + it('an oversized body is an answer too', async () => { + // The service replied, and replied too much. That is a bug upstream, not a + // reason to stop asking — and the caller already falls back for this one. + for (let i = 0; i < 10; i++) { + stubStreamingFetch([BIG_BODY, BIG_BODY]); + await expect(trekPlacesSearch('x')).rejects.toThrow('too large'); + } + stubFetch({ query: 'x', results: [PLACE] }); + await expect(trekPlacesSearch('x')).resolves.toHaveLength(1); + }); +}); + describe('getJson request shape', () => { it('carries an abort signal on every call, and aborts when the answer never comes', async () => { // The repo rule is that no outbound fetch runs without a timeout. Nothing diff --git a/server/vitest.config.ts b/server/vitest.config.ts index 8a6bd6d85..16e5fe998 100644 --- a/server/vitest.config.ts +++ b/server/vitest.config.ts @@ -103,6 +103,14 @@ export default defineConfig({ // close. Set with a few points of slack against the Linux/Windows drift. 'src/nest/place-enrichment/**/*.ts': { statements: 96, branches: 95, functions: 94, lines: 97 }, 'src/nest/place-photos/**/*.ts': { statements: 87, branches: 79, functions: 72, lines: 89 }, + // New domain in this change. Measured over its own suites at + // 98.3/89.8/96.2/100, and pinned well under that on purpose: the + // subset run does not load the controller the way the full run does, + // so the real figure is not this one. Regenerate with + // scripts/coverage-thresholds.mjs after a full run. What matters now is + // that it is off the 80 catch-all, where a new domain could shed twenty + // points without anything noticing. + 'src/nest/place-shadow/**/*.ts': { statements: 90, branches: 80, functions: 90, lines: 95 }, 'src/nest/places/**/*.ts': { statements: 91, branches: 82, functions: 96, lines: 94 }, 'src/nest/platform/**/*.ts': { statements: 99, branches: 99, functions: 99, lines: 99 }, 'src/nest/plugins/**/*.ts': { statements: 86, branches: 81, functions: 78, lines: 89 },