diff --git a/client/src/api/client.ts b/client/src/api/client.ts index 3d65ccb94..07f4c0ac3 100644 --- a/client/src/api/client.ts +++ b/client/src/api/client.ts @@ -931,11 +931,13 @@ export const mapsApi = { details: (placeId: string, lang?: string) => apiClient.get(`/maps/details/${encodeURIComponent(placeId)}`, { params: { lang } }).then(r => checkInDev(mapsPlaceDetailsResultSchema, r.data, 'maps.details')), // Pictures and a description for a place that is being looked at but not yet // saved. Fans out to several providers server-side, so it takes a signal and - // the caller is expected to abort it when the selection changes. + // the caller is expected to abort it when the selection changes, and a longer + // timeout than the global 8s — a cold Wikimedia connection alone can eat that. placeEnrichment: ( body: { placeId?: string; lat: number; lng: number; name: string; lang?: string }, signal?: AbortSignal, - ) => apiClient.post('/maps/enrichment', body, { signal }).then(r => checkInDev(mapsPlaceEnrichmentResultSchema, r.data, 'maps.placeEnrichment')), + ) => apiClient.post('/maps/enrichment', body, { signal, timeout: 25000 }) + .then(r => checkInDev(mapsPlaceEnrichmentResultSchema, r.data, 'maps.placeEnrichment')), // Author + licence for a picture already stored in the photo cache, keyed by // the cache key embedded in its proxy URL. Local read, no provider call. placePhotoCredit: (key: string) => diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index df32db55c..e8cc2c438 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -130,6 +130,11 @@ interface GooglePlaceDetails extends GooglePlaceResult { // of places cannot monopolise the event loop or trigger external API rate limits. // Module-scoped ON PURPOSE (permissions-cache precedent): the bridge instance and // the DI singleton must share one limiter, one POI cache and one call counter. +// Wikimedia is normally well under a second, but a cold TLS handshake from a +// fresh container has been seen at eight. Enrichment answers a live dialog, so +// a slow provider is dropped rather than waited out. +const WIKI_TIMEOUT_MS = 6000; + const MAX_CONCURRENT_PHOTO_FETCHES = 5; let photoFetchActive = 0; const photoFetchQueue: Array<() => void> = []; @@ -624,7 +629,12 @@ export class MapsService { iiurlwidth: '400', }); try { - const res = await fetch(`https://commons.wikimedia.org/w/api.php?${params}`, { headers: { 'User-Agent': UA } }); + const res = await fetch(`https://commons.wikimedia.org/w/api.php?${params}`, { + headers: { 'User-Agent': UA }, + // A hanging provider must not hold the whole enrichment request open; + // no pictures is a fine answer, a request that never returns is not. + signal: AbortSignal.timeout(WIKI_TIMEOUT_MS), + }); if (!res.ok) return []; const data = (await res.json()) as { query?: { pages?: Record } }; const pages = data.query?.pages; @@ -681,6 +691,7 @@ export class MapsService { try { const res = await fetch(`https://${parsed.lang}.wikipedia.org/w/api.php?${params}`, { headers: { 'User-Agent': UA }, + signal: AbortSignal.timeout(WIKI_TIMEOUT_MS), }); if (!res.ok) return null; const data = (await res.json()) as { query?: { pages?: Record } }; diff --git a/server/src/nest/place-enrichment/place-enrichment.service.ts b/server/src/nest/place-enrichment/place-enrichment.service.ts index 925464ab6..be229e92b 100644 --- a/server/src/nest/place-enrichment/place-enrichment.service.ts +++ b/server/src/nest/place-enrichment/place-enrichment.service.ts @@ -123,52 +123,61 @@ export class PlaceEnrichmentService { placeId: string, req: MapsPlaceEnrichmentRequest, ): Promise { - const out: PlacePhotoCandidate[] = []; - - // Google first when it is available at all — its pictures are of the place - // itself, while Commons geosearch finds whatever was photographed nearby. const apiKey = this.maps.getMapsKey(userId); - if (apiKey && !this.maps.photosDisabled() && isGooglePlaceId(placeId)) { - const refs = await this.maps.fetchGooglePhotoRefs(placeId, apiKey, GOOGLE_CAP); - for (const ref of refs) { - const key = candidateKey(placeId, out.length); - const url = await this.storeCandidate(key, creditLine(ref.attribution, null), () => - this.maps.fetchGooglePhotoBytes(ref.name, apiKey), - ); - if (!url) continue; - out.push({ - key, - url, - attribution: ref.attribution, - // Google grants no reusable licence for these; the author line it - // hands back is all we may show, so the rest stays honestly empty. - license: null, - licenseUrl: null, - sourceUrl: null, - source: 'google', - }); - } - } + const wantsGoogle = !!apiKey && !this.maps.photosDisabled() && isGooglePlaceId(placeId); - const commons = await this.maps.fetchCommonsCandidates(req.lat, req.lng, COMMONS_CAP); - for (const candidate of commons) { - const key = candidateKey(placeId, out.length); - const url = await this.storeCandidate(key, creditLine(candidate.attribution, candidate.license), () => - this.fetchRemoteBytes(candidate.photoUrl), - ); - if (!url) continue; - out.push({ - key, - url, + // Ask both providers at once — one listing call each, and they know nothing + // about one another. + const [googleRefs, commons] = await Promise.all([ + wantsGoogle ? this.maps.fetchGooglePhotoRefs(placeId, apiKey!, GOOGLE_CAP) : Promise.resolve([]), + this.maps.fetchCommonsCandidates(req.lat, req.lng, COMMONS_CAP), + ]); + + // Google first — its pictures are of the place itself, while Commons + // geosearch finds whatever happens to have been photographed nearby. + const pending: PlacePhotoCandidate[] = [ + ...googleRefs.map((ref) => ({ + key: '', + url: '', + attribution: ref.attribution, + // Google grants no reusable licence for these; the author line it hands + // back is all we may show, so the rest stays honestly empty. + license: null, + licenseUrl: null, + sourceUrl: null, + source: 'google' as const, + })), + ...commons.map((candidate) => ({ + key: '', + url: '', attribution: candidate.attribution, license: candidate.license, licenseUrl: candidate.licenseUrl, sourceUrl: candidate.sourceUrl, - source: 'wikimedia', - }); - } + source: 'wikimedia' as const, + })), + ]; - return out; + // Download the bytes concurrently. Doing this in a loop meant a place with + // five Commons hits spent five round trips plus five decodes in sequence, + // which pushed the whole request past the client's request timeout. The + // shared slot limiter still caps how many run at once. + const results = await Promise.all( + pending.map(async (candidate, index) => { + const key = candidateKey(placeId, index); + const fetchBytes = + candidate.source === 'google' + ? () => this.maps.fetchGooglePhotoBytes(googleRefs[index].name, apiKey!) + : () => this.fetchRemoteBytes(commons[index - googleRefs.length].photoUrl); + const url = await this.storeCandidate(key, creditLine(candidate.attribution, candidate.license), fetchBytes); + // The index is fixed before any download, so a picture that fails to + // load leaves a gap rather than renumbering the ones after it — the key + // has to keep pointing at the same file across requests. + return url ? { ...candidate, key, url } : null; + }), + ); + + return results.filter((candidate): candidate is PlacePhotoCandidate => candidate !== null); } /** diff --git a/server/tests/unit/nest/place-enrichment.service.test.ts b/server/tests/unit/nest/place-enrichment.service.test.ts index 629ee14de..c21c76ac0 100644 --- a/server/tests/unit/nest/place-enrichment.service.test.ts +++ b/server/tests/unit/nest/place-enrichment.service.test.ts @@ -219,9 +219,11 @@ describe('collectPhotos', () => { const out = await make(maps, cacheStub()).enrich(1, REQ); - // The survivor takes index 0 — keys number the strip, not the source list. + // The survivor keeps index 1 — keys number the provider's list, so a picture + // that fails leaves a gap instead of renumbering the ones behind it. Keys + // have to point at the same file on the next request. expect(out.photos).toHaveLength(1); - expect(out.photos[0].key).toBe('ChIJmuseum~p0'); + expect(out.photos[0].key).toBe('ChIJmuseum~p1'); }); it('ENRICH-010: treats an empty body and a throwing download as a miss', async () => { @@ -274,7 +276,51 @@ describe('collectPhotos', () => { const out = await make(maps, cacheStub()).enrich(1, REQ); expect(out.photos).toHaveLength(1); - expect(out.photos[0]).toMatchObject({ key: 'ChIJmuseum~p0', attribution: 'Cara' }); + expect(out.photos[0]).toMatchObject({ key: 'ChIJmuseum~p1', attribution: 'Cara' }); + }); + + it('ENRICH-012c: downloads the pictures concurrently, not one after another', async () => { + // Five sequential round trips plus five decodes pushed the request past the + // client's timeout, which is what made the column show an error every time. + let active = 0; + let peak = 0; + const maps = mapsStub({ + fetchCommonsCandidates: vi.fn(async () => [commonsCandidate(), commonsCandidate(), commonsCandidate()]), + }); + mockSafeFetchFollow.mockImplementation(async () => { + active++; + peak = Math.max(peak, active); + await new Promise((resolve) => setTimeout(resolve, 5)); + active--; + return { ok: true, arrayBuffer: async () => new Uint8Array([1, 2, 3]).buffer }; + }); + + const out = await make(maps, cacheStub()).enrich(1, REQ); + + expect(out.photos).toHaveLength(3); + expect(peak).toBeGreaterThan(1); + }); + + it('ENRICH-012d: asks both providers at the same time', async () => { + const order: string[] = []; + const maps = mapsStub({ + getMapsKey: vi.fn(() => 'key'), + fetchGooglePhotoRefs: vi.fn(async () => { + order.push('google:start'); + await new Promise((resolve) => setTimeout(resolve, 10)); + order.push('google:end'); + return []; + }), + fetchCommonsCandidates: vi.fn(async () => { + order.push('commons:start'); + return []; + }), + }); + + await make(maps, cacheStub()).enrich(1, REQ); + + // Commons starts before Google finishes; the listings do not queue. + expect(order.indexOf('commons:start')).toBeLessThan(order.indexOf('google:end')); }); it('ENRICH-013: falls back to a coordinate pseudo-id when the place has none', async () => {