From e1ac5ac270be4cf7ce63d5cf19c7afc4ade72d2e Mon Sep 17 00:00:00 2001 From: Sameh Khamis Date: Thu, 1 Oct 2026 16:10:43 -0700 Subject: [PATCH] fix: cancel shared place lookups nobody is waiting for The place cache shared one lookup between callers but started it without a signal or deadline, so a cancelled request kept running and stayed cached. A shared lookup now has its own deadline, is cancelled when every caller has given up, and only a successful answer is kept. --- src/tools/places.js | 88 ++++++++++++++++++++++++++------------- src/tools/places.test.mjs | 56 +++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 29 deletions(-) diff --git a/src/tools/places.js b/src/tools/places.js index 97b4ce8f1..4a715f389 100644 --- a/src/tools/places.js +++ b/src/tools/places.js @@ -22,53 +22,83 @@ export function createGeocodePlaceService({ fetchImpl = (...args) => globalThis.fetch(...args), cacheSize = 200, cacheMs = 10 * 60_000, + timeoutMs = 15_000, now = () => Date.now(), } = {}) { // Recent answers by name, so tools that combine others resolve a place to - // the same point once instead of once per section. + // the same point once instead of once per section. A pending lookup is + // shared by its callers: it has its own deadline, is cancelled when every + // caller has given up, and only a successful answer stays cached. const cache = new Map(); - async function lookup(name) { + async function lookup(name, signal) { const query = new URLSearchParams({ q: name }); - const response = await fetchImpl(`/api/geocode?${query}`); + const response = await fetchImpl(`/api/geocode?${query}`, { signal }); if (!response.ok) throw new Error(`Geocode HTTP ${response.status}`); const payload = await response.json(); return placeFromGeocodeResult(payload?.results?.[0]); } + function start(key, name) { + const controller = new AbortController(); + const entry = { + at: now(), + controller, + waiters: 0, + settled: false, + place: lookup( + name, + AbortSignal.any([controller.signal, AbortSignal.timeout(timeoutMs)]), + ), + }; + entry.place.then( + () => { + entry.settled = true; + }, + () => { + entry.settled = true; + if (cache.get(key) === entry) cache.delete(key); + }, + ); + cache.set(key, entry); + while (cache.size > cacheSize) cache.delete(cache.keys().next().value); + return entry; + } + function join(key, entry, signal) { + if (!signal) { + // A caller that cannot cancel keeps the lookup alive. + entry.waiters = Infinity; + return entry.place; + } + signal.throwIfAborted(); + entry.waiters += 1; + return new Promise((resolve, reject) => { + const leave = () => { + entry.waiters -= 1; + reject(signal.reason); + if (entry.waiters <= 0 && !entry.settled) { + entry.controller.abort(signal.reason); + if (cache.get(key) === entry) cache.delete(key); + } + }; + signal.addEventListener('abort', leave, { once: true }); + entry.place + .then(resolve, reject) + .finally(() => signal.removeEventListener('abort', leave)); + }); + } return { async resolve(name, { signal } = {}) { signal?.throwIfAborted(); const key = name.trim().toLowerCase(); - const hit = cache.get(key); - if (hit && now() - hit.at < cacheMs) { + let entry = cache.get(key); + if (entry && now() - entry.at < cacheMs) { cache.delete(key); - cache.set(key, hit); - return waitFor(hit.place, signal); - } - const place = lookup(name); - const entry = { at: now(), place }; - cache.set(key, entry); - while (cache.size > cacheSize) cache.delete(cache.keys().next().value); - place.catch(() => { - if (cache.get(key) === entry) cache.delete(key); - }); - return waitFor(place, signal); + cache.set(key, entry); + } else entry = start(key, name); + return join(key, entry, signal); }, }; } -/** Settle with `promise`, or reject when `signal` aborts first. */ -function waitFor(promise, signal) { - if (!signal) return promise; - signal.throwIfAborted(); - return new Promise((resolve, reject) => { - const abort = () => reject(signal.reason); - signal.addEventListener('abort', abort, { once: true }); - promise - .then(resolve, reject) - .finally(() => signal.removeEventListener('abort', abort)); - }); -} - /** Convert one geocode result to a place with bounds, or null. */ export function placeFromGeocodeResult(result) { const location = result?.geometry?.location; diff --git a/src/tools/places.test.mjs b/src/tools/places.test.mjs index 63fdeb28a..f4391269a 100644 --- a/src/tools/places.test.mjs +++ b/src/tools/places.test.mjs @@ -37,3 +37,59 @@ test('place lookups are cached by name for a while, and failures are not', async aborted.abort(); await assert.rejects(places.resolve('Tokyo', { signal: aborted.signal })); }); + +test('a lookup every caller abandoned is cancelled and not reused', async () => { + const signals = []; + const places = createGeocodePlaceService({ + fetchImpl: (url, { signal }) => { + signals.push(signal); + if (signals.length === 1) + return new Promise((resolve, reject) => + signal.addEventListener('abort', () => reject(signal.reason)), + ); + return Promise.resolve(Response.json(answer)); + }, + }); + const caller = new AbortController(); + const first = places.resolve('Tokyo', { signal: caller.signal }); + caller.abort(); + await assert.rejects(first); + assert.equal(signals[0].aborted, true); + const again = await places.resolve('Tokyo'); + assert.deepEqual(again.point, { lat: 35.68, lon: 139.76 }); + assert.equal(signals.length, 2); +}); + +test('a shared lookup outlives one cancelled caller, and times out on its own', async () => { + let release; + const signals = []; + const places = createGeocodePlaceService({ + timeoutMs: 50, + fetchImpl: (url, { signal }) => { + signals.push(signal); + if (url.includes('Slow')) + return new Promise((resolve, reject) => + signal.addEventListener('abort', () => reject(signal.reason)), + ); + return new Promise((resolve) => { + release = () => resolve(Response.json(answer)); + }); + }, + }); + const leaving = new AbortController(); + const gone = places.resolve('Tokyo', { signal: leaving.signal }); + const staying = places.resolve('Tokyo', { + signal: new AbortController().signal, + }); + leaving.abort(); + await assert.rejects(gone); + assert.equal(signals[0].aborted, false); + release(); + assert.deepEqual((await staying).point, { lat: 35.68, lon: 139.76 }); + await assert.rejects( + places.resolve('Slow'), + (error) => error.name === 'TimeoutError', + ); + await places.resolve('Slow').catch(() => {}); + assert.equal(signals.length, 3, 'a timed-out lookup is not cached'); +});