From f8795ff426d4fc2158ebb04f497a6a1e20f6ce01 Mon Sep 17 00:00:00 2001 From: quanticsoul4772 <71096528+quanticsoul4772@users.noreply.github.com> Date: Thu, 29 Oct 2026 07:26:15 -0700 Subject: [PATCH] fix(routes): report an upstream routing outage as unavailable When the OSRM routing service failed (a 5xx, a 403 block, a 404), answered with a body that was not JSON, or did not answer at all (timeout, network error), /api/route returned HTTP 200 with 'no route found' or 'route proxy error'. plan_route turned that into invalid_arguments ("No walk route from ...: no route found"), blaming the caller for an outage, and the directions layer reported "No route found between A and B". The proxy now answers all of these with 502 {ok:false, error:'routing service unavailable'}: a rejected upstream fetch, any failure status but 400, a non-JSON content type, and a body read that fails or does not parse. An exception in the proxy's own code after that point is still a 200 'route proxy error'. createRouteService throws for a 5xx before reading the body's ok:false, so plan_route reports unavailable, and the directions source treats any 5xx as "Routing unavailable (HTTP n)", also when the failure page in front of the proxy is not JSON. OSRM's own refusal (a 400 such as NoRoute, or a 200 without a route) still reads "no route found". The 12 s upstream timeout now starts when the request leaves rather than before it waits for an outbound slot, so a wait in the proxy's own queue (up to about 8 s at the 1 s spacing) is not reported as an outage. Co-Authored-By: Claude Opus 5.5 --- docs/CURRENT-STATE.md | 12 ++- server/providers/places/routes.js | 72 +++++++++++++---- src/layers/directions/source.js | 12 ++- src/layers/directions/source.test.mjs | 12 +++ src/tooling/placeProviders.test.mjs | 108 +++++++++++++++++++++++--- src/tools/places.js | 5 +- src/tools/queries/places.test.mjs | 36 +++++++++ 7 files changed, 223 insertions(+), 34 deletions(-) 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),