diff --git a/docs/CURRENT-STATE.md b/docs/CURRENT-STATE.md index 8a8a5aa1d..178369f66 100644 --- a/docs/CURRENT-STATE.md +++ b/docs/CURRENT-STATE.md @@ -4375,7 +4375,15 @@ service's usage policy states; past a bounded queue the answer is a 429 with a `Retry-After` rather than a queue that grows until everything times out. The upstream host is pinned and a redirect is refused rather than followed, and a rate limit from the routing service is reported as one instead of as "no route -found". `src/data/routeSteps.js` turns OSRM maneuver type / modifier / exit / +found". So is an outage: any failure status but 400 (a 5xx, a 403 block, a +404), no answer (a 12 s timeout counted from when the request leaves, not +while it waits for an outbound slot; a network error) or a body that is not +JSON answers +`502 routing service unavailable`, which `plan_route` +reports as `unavailable` and the layer as `Routing unavailable (HTTP 502)`; +only OSRM's own refusal (a 400, or a 200 without a route) reads "no route +found". +`src/data/routeSteps.js` turns OSRM maneuver type / modifier / exit / road name into one plain-English sentence per decision, folding `exit roundabout` steps into the roundabout they leave, and caps one route at 200 maneuvers — a route with more is reported as cut off, in the row summary @@ -5694,7 +5702,7 @@ apply to `public/` assets or built-preview serving. - `/api/google/text-search` keeps the Google key server-side for view-biased Places recovery used by annotation resolution. - `/api/overpass` is bounded by body/response caps, per-client/global rate limits, concurrency limits, operator-configured upstreams, in-flight dedupe, cache bounds, and static validation that every selector is spatially bounded. - `/api/military-installations` uses an independent limiter with the same 90-per-client/300-global one-minute bounds, so viewport installation refreshes never consume `/api/overpass` annotation capacity. -- `/api/route` proxies bounded OSRM route requests for annotation routes, with profile allowlisting, distance caps, response caps, caching, and sanitized "no route found" errors. +- `/api/route` proxies bounded OSRM route requests for annotation routes, with profile allowlisting, distance caps, response caps, caching, and sanitized "no route found" errors; a routing-service outage answers 502 instead. - Track endpoints: `/api/vessels/track?mmsi=` (server-accumulated ring buffers; sub-route handled before the rows snapshot), `/api/flights/track?icao24=` (OAuth, 60s cache, sanitized errors, independent OpenSky credit bucket), `/api/military/track?hex=` (400 for an invalid hex, otherwise `503 history_unavailable` with `Retry-After: 1800` and `Cache-Control: no-store`, no upstream request; military tracking uses its locally accumulated trail). - Realtime debug logs redact API keys, bearer tokens, client secrets, and image data URLs before writing to disk; request bodies are size-capped. - `/api/realtime/debug-log` enforces an always-on 120-per-client/400-global one-minute limiter (not the opt-in `GEV_RATELIMIT_OPENAI_PER_MIN` bucket the cost-bearing OpenAI routes share), appends asynchronously through a serialized queue, and rotates `realtime-conversations.jsonl` at 32 MB keeping one prior generation, so the sink is bounded at twice that regardless of session length. A malformed record answers 400 and a failed write 500, both with a fixed message. diff --git a/server/providers/places/routes.js b/server/providers/places/routes.js index 42e24ad32..34e64d20d 100644 --- a/server/providers/places/routes.js +++ b/server/providers/places/routes.js @@ -72,6 +72,17 @@ class UpstreamRateLimitError extends Error { } } +/** + * Thrown when the routing service failed, did not answer (timeout, network + * error, refused redirect) or did not answer with JSON. + */ +class UpstreamUnavailableError extends Error { + constructor() { + super('routing service unavailable'); + this.name = 'UpstreamUnavailableError'; + } +} + /** * Run one outbound request no sooner than the shared gate allows. * @template T @@ -167,18 +178,26 @@ async function fetchRoute({ `${base.replace(/\/$/, '')}/route/v1/${osrmProfile}/${coords}` + `?overview=full&geometries=geojson&alternatives=false&steps=${withSteps ? 'true' : 'false'}`; const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), 12000); + let timer; let osrm; try { - const upstreamRes = await throughUpstreamGate(() => - fetchImpl(upstream, { - signal: controller.signal, - // The endpoint is configured above; a redirect is the one way out of - // it, so it is refused rather than followed. - redirect: 'error', - headers: { 'User-Agent': 'gods-eye-view/dev (local)' }, - }), - ); + const upstreamRes = await throughUpstreamGate(async () => { + // The 12 s budget starts when the request leaves, not while it waits + // for an outbound slot, so a wait in our own queue is never reported + // as the routing service being unavailable. + timer = setTimeout(() => controller.abort(), 12000); + try { + return await fetchImpl(upstream, { + signal: controller.signal, + // The endpoint is configured above; a redirect is the one way out of + // it, so it is refused rather than followed. + redirect: 'error', + headers: { 'User-Agent': 'gods-eye-view/dev (local)' }, + }); + } catch { + throw new UpstreamUnavailableError(); + } + }); if (upstreamRes.status === 429) { await cancelBody(upstreamRes); const retryAfter = Number(upstreamRes.headers.get('retry-after')); @@ -186,20 +205,31 @@ async function fetchRoute({ Number.isFinite(retryAfter) && retryAfter > 0 ? retryAfter : 5, ); } - if (!upstreamRes.ok) { + // OSRM answers 400 for a request it cannot route (NoRoute, NoSegment); + // any other failure status (a 5xx, a 403 block, a 404) is the service's. + if (upstreamRes.status === 400) { await cancelBody(upstreamRes); return { payload: null, error: 'no route found' }; } + if (!upstreamRes.ok) { + await cancelBody(upstreamRes); + throw new UpstreamUnavailableError(); + } const ctype = upstreamRes.headers.get('content-type') || ''; if (!ctype.includes('json')) { await cancelBody(upstreamRes); - return { payload: null, error: 'no route found' }; + throw new UpstreamUnavailableError(); + } + // A body cut off by the timeout or the network, an oversized one and one + // that is not JSON are all the service's failure; an exception after this + // point is ours and stays a "route proxy error". + try { + osrm = JSON.parse( + await readResponseTextCapped(upstreamRes, ROUTE_MAX_RESPONSE_BYTES), + ); + } catch { + throw new UpstreamUnavailableError(); } - const text = await readResponseTextCapped( - upstreamRes, - ROUTE_MAX_RESPONSE_BYTES, - ); - osrm = JSON.parse(text); } finally { clearTimeout(timer); } @@ -238,6 +268,10 @@ export function installRouteMiddleware( }); res.end(JSON.stringify({ ok: false, error: msg })); }; + const unavailable = (msg) => { + res.writeHead(502, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ ok: false, error: msg })); + }; try { if (!_routeRateLimiter(clientKey(req))) { rateLimited('rate limited', 5); @@ -350,6 +384,10 @@ export function installRouteMiddleware( upstreamError.retryAfterSec, ); } + if (upstreamError instanceof UpstreamUnavailableError) { + // Same reason: an outage says nothing about whether a route exists. + return unavailable(upstreamError.message); + } throw upstreamError; } if (error || !payload) return fail(error || 'no route found'); diff --git a/src/layers/directions/source.js b/src/layers/directions/source.js index 2766f83d4..52ffc0888 100644 --- a/src/layers/directions/source.js +++ b/src/layers/directions/source.js @@ -21,7 +21,13 @@ export function createDirectionsSource({ signal, headers: { Accept: 'application/json' }, }); - const payload = await response.json(); + // A failure page in front of the proxy need not be JSON. + let payload = null; + try { + payload = await response.json(); + } catch (error) { + if (response.ok) throw error; + } signal?.throwIfAborted(); // Failures keep their English message and carry the HTTP status and the // server's refusal text, so the layer can show them in the language. @@ -39,7 +45,9 @@ export function createDirectionsSource({ { status: 429, refusal }, ); } - if (!response.ok && !payload?.error) + // A 5xx is a routing outage whatever its body says; only the proxy's + // refusals carry an error text worth showing. + if (response.status >= 500 || (!response.ok && !payload?.error)) throw Object.assign( new Error(`Routing unavailable (HTTP ${response.status})`), { status: response.status }, diff --git a/src/layers/directions/source.test.mjs b/src/layers/directions/source.test.mjs index 824b1221b..467b8ee52 100644 --- a/src/layers/directions/source.test.mjs +++ b/src/layers/directions/source.test.mjs @@ -41,3 +41,15 @@ test('route failures are surfaced without fallback requests', async () => { assert.equal(calls, 1); } }); + +test('a routing outage stays an outage when its body carries an error', async () => { + const source = createDirectionsSource({ fetchImpl: async () => + Response.json({ ok: false, error: 'routing service unavailable' }, { status: 502 }) }); + await assert.rejects(source.getRoute(query), { status: 502, message: 'Routing unavailable (HTTP 502)' }); +}); + +test('a routing outage page that is not JSON still reads as an outage', async () => { + const source = createDirectionsSource({ fetchImpl: async () => + new Response('Bad Gateway', { status: 502, headers: { 'content-type': 'text/html' } }) }); + await assert.rejects(source.getRoute(query), { status: 502, message: 'Routing unavailable (HTTP 502)' }); +}); diff --git a/src/tooling/placeProviders.test.mjs b/src/tooling/placeProviders.test.mjs index d533b7fb9..1540d455b 100644 --- a/src/tooling/placeProviders.test.mjs +++ b/src/tooling/placeProviders.test.mjs @@ -236,23 +236,107 @@ test('OSRM routing preserves aliases, cache, span guards and upstream failure be assert.equal((await request(route, url)).body.error, error); assert.equal(calls, 1); now += 600001; - t.mock.method( - globalThis, - 'fetch', - async () => new Response('offline', { status: 503 }), + // OSRM's own "no route" is a 400 with a JSON code, and stays one. + t.mock.method(globalThis, 'fetch', async () => + Response.json( + { code: 'NoRoute', message: 'Impossible route between points' }, + { status: 400 }, + ), ); - assert.deepEqual((await request(route, query)).body, { - ok: false, - error: 'no route found', - }); - t.mock.method( - globalThis, - 'fetch', - async () => new Response('x', { headers: { 'content-type': 'text/html' } }), + const noRoute = await request(route, query); + assert.equal(noRoute.statusCode, 200); + assert.deepEqual(noRoute.body, { ok: false, error: 'no route found' }); + t.mock.method(globalThis, 'fetch', async () => + Response.json({ code: 'NoRoute', routes: [] }), ); assert.equal((await request(route, query)).body.error, 'no route found'); }); +test('a routing outage is reported as one, not as "no route found"', async (t) => { + const route = '/api/route'; + const query = '?profile=car&coords=-97,30;-97.01,30.01'; + for (const response of [ + () => new Response('offline', { status: 503 }), + () => new Response('blocked', { status: 403 }), + () => new Response('x', { headers: { 'content-type': 'text/html' } }), + () => + new Response('', { + headers: { 'content-type': 'application/json' }, + }), + () => Promise.reject(new TypeError('fetch failed')), + () => Promise.reject(new DOMException('aborted', 'AbortError')), + ]) { + _resetRouteUpstreamForTest(); + const request = install(installRouteMiddleware); + t.mock.method(globalThis, 'fetch', async () => response()); + const result = await request(route, query); + assert.equal(result.statusCode, 502); + assert.deepEqual(result.body, { + ok: false, + error: 'routing service unavailable', + }); + } +}); + +test('callers sharing one failed upstream call all hear the outage, and it is not cached', async (t) => { + _resetRouteUpstreamForTest(); + _setRouteUpstreamIntervalForTest(0); + t.after(() => _resetRouteUpstreamForTest()); + const request = install(installRouteMiddleware); + let calls = 0; + let release; + const gate = new Promise((resolve) => { + release = resolve; + }); + t.mock.method(globalThis, 'fetch', async () => { + calls += 1; + await gate; + return new Response('offline', { status: 503 }); + }); + const route = '/api/route'; + const query = '?profile=car&coords=-97,30;-97.01,30.01'; + const all = [ + request(route, `${query}&steps=1`), + request(route, `${query}&steps=1`), + request(route, query), + ]; + await Promise.resolve(); + release(); + for (const result of await Promise.all(all)) { + assert.equal(result.statusCode, 502); + assert.equal(result.body.error, 'routing service unavailable'); + } + assert.equal(calls, 1); + assert.equal((await request(route, query)).statusCode, 502); + assert.equal(calls, 2, 'the next request asks the routing service again'); +}); + +test('a wait for an outbound slot does not count toward the upstream timeout', async (t) => { + _resetRouteUpstreamForTest(); + // The second request waits 13 s for its slot, longer than the 12 s budget. + _setRouteUpstreamIntervalForTest(13_000); + t.after(() => _resetRouteUpstreamForTest()); + t.mock.timers.enable({ apis: ['setTimeout', 'Date'], now: Date.now() }); + const request = install(installRouteMiddleware); + t.mock.method(globalThis, 'fetch', async (_url, { signal }) => { + signal.throwIfAborted(); + return Response.json(osrmRouteWithSteps(2)); + }); + const first = request( + '/api/route', + '?profile=car&coords=-97,30;-97.01,30.01', + ); + const second = request( + '/api/route', + '?profile=foot&coords=-97,30;-97.01,30.01', + ); + assert.equal((await first).body.ok, true); + t.mock.timers.tick(13_000); + const result = await second; + assert.equal(result.statusCode, 200); + assert.equal(result.body.ok, true); +}); + /** One OSRM route with `count` maneuvers, for the step tests. */ function osrmRouteWithSteps(count) { return { diff --git a/src/tools/places.js b/src/tools/places.js index 4a715f389..8d67ec5d1 100644 --- a/src/tools/places.js +++ b/src/tools/places.js @@ -189,7 +189,10 @@ export function createRouteService({ } catch { /* the status below is authoritative */ } - if (response.status === 429) throw httpFailure(response, 'Routing'); + // A rate limit or an outage (5xx) is the routing service's failure, not + // a missing route, whatever the body says. + if (response.status === 429 || response.status >= 500) + throw httpFailure(response, 'Routing'); if (payload?.ok === false || response.status === 400) return { ok: false, error: payload?.error || 'no route found' }; if (!response.ok) throw httpFailure(response, 'Routing'); diff --git a/src/tools/queries/places.test.mjs b/src/tools/queries/places.test.mjs index 89cad9ba8..7da838b45 100644 --- a/src/tools/queries/places.test.mjs +++ b/src/tools/queries/places.test.mjs @@ -1,6 +1,7 @@ import assert from 'node:assert/strict'; import test from 'node:test'; import { composeCatalog, coreTools } from '../index.js'; +import { createMcpServer } from '../mcp/index.js'; import { createPlaceSearchService, createRouteService } from '../places.js'; const place = (id, latitude, longitude, extra = {}) => ({ @@ -249,6 +250,41 @@ test('the place and route services read the app routes and translate failures', ); }); +test('a routing outage is reported as unavailable, not as invalid arguments', async () => { + for (const fetchImpl of [ + async () => + Response.json( + { ok: false, error: 'routing service unavailable' }, + { status: 502 }, + ), + async () => { + throw new TypeError('fetch failed'); + }, + ]) { + const catalog = composeCatalog({ + tools: coreTools, + services: { routing: createRouteService({ fetchImpl }), places }, + }); + const server = createMcpServer({ + catalog, + name: 'test', + version: '1.0.0', + }); + const { result } = await server.handle({ + jsonrpc: '2.0', + id: 1, + method: 'tools/call', + params: { + name: 'plan_route', + arguments: { from: { place: 'Austin' }, to: { place: 'Dallas' } }, + }, + }); + assert.equal(result.isError, true); + assert.equal(result.structuredContent.error, 'unavailable'); + assert.doesNotMatch(result.content[0].text, /No walk route/); + } +}); + test('place answers say when the search limits may have left places out', async () => { const five = Array.from({ length: 5 }, (_, index) => place(`p${index}`, 30.27, -97.74),