From e9742e93224d85084b6ef3beb1dba6af802b2ad7 Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 18 Sep 2026 13:17:51 +0200 Subject: [PATCH] fix(roadtrip): a refused run asks again one pair at a time One request per day is thrifty, and the price of it is that a refusal is never local. With the usual cause gone the shape of the failure is still there, so a run the router refuses outright now asks for its legs one at a time instead of leaving the day blank. The pairs are enqueued rather than awaited, so they keep the spacing of every other request, and a rate limit is never split: answering a host that asked for less traffic with four more requests is the opposite of backing off. A refusal the host meant skips the two retries, because the same coordinates earn the same 400. Seam legs are merged under the day's own runs now rather than over them. legsByDay is replaced whole on every routing round while seamLegs only ever grows, so a pair that used to sit on a day boundary and now sits inside one day kept an answer nobody re-asks for, shaped by the vias of a stop that has moved. --- .../Roadtrip/useRoadtripRoutes.test.ts | 36 ++++ .../components/Roadtrip/useRoadtripRoutes.ts | 183 +++++++++++------- 2 files changed, 147 insertions(+), 72 deletions(-) diff --git a/client/src/components/Roadtrip/useRoadtripRoutes.test.ts b/client/src/components/Roadtrip/useRoadtripRoutes.test.ts index ed676cc62..4227dcde8 100644 --- a/client/src/components/Roadtrip/useRoadtripRoutes.test.ts +++ b/client/src/components/Roadtrip/useRoadtripRoutes.test.ts @@ -278,6 +278,42 @@ describe('useRoadtripRoutes', () => { expect(result.current.totalDistance).toBe(100000) }) + it('FE-ROADTRIP-ROUTES-028: a day the router refuses outright is asked for again one pair at a time', async () => { + const days = [day(1, 1)] + const stops: StopSpec[] = [{ id: 1, at: HAMBURG }, { id: 2, at: LUENEBURG }, { id: 3, at: BERLIN }] + // The Padirac case: one stop in the middle the router will not turn round at, and + // what comes back is a refusal of the WHOLE chain rather than of that one leg. + calculateRouteWithLegs + .mockRejectedValueOnce(new RoutingRefusedError(400, null)) + .mockResolvedValue(routed(1)) + + const { result } = renderHook(() => useRoadtripRoutes(7, days, map(1, stops))) + await waitFor(() => expect(result.current.loading).toBe(false), { timeout: 5000 }) + + // The run once, refused, then a request per pair, and no retry of the run, because + // the same coordinates earn the same refusal. + expect(calculateRouteWithLegs).toHaveBeenCalledTimes(3) + expect(calculateRouteWithLegs.mock.calls[1][0]).toHaveLength(2) + expect(calculateRouteWithLegs.mock.calls[2][0]).toHaveLength(2) + expect(result.current.days[0].legs.filter(Boolean)).toHaveLength(2) + expect(result.current.totalDistance).toBe(200000) + }) + + it('FE-ROADTRIP-ROUTES-029: a rate limit is waited out, never split into more requests', async () => { + vi.useFakeTimers() + const days = [day(1, 1)] + const stops: StopSpec[] = [{ id: 1, at: HAMBURG }, { id: 2, at: LUENEBURG }, { id: 3, at: BERLIN }] + calculateRouteWithLegs.mockRejectedValue(new RoutingRefusedError(429, null)) + + const { result } = renderHook(() => useRoadtripRoutes(7, days, map(1, stops))) + await act(async () => { await vi.advanceTimersByTimeAsync(20000) }) + + // Three attempts at the run and nothing after them: answering a host that asked for + // less traffic with two more requests would be the opposite of backing off. + expect(calculateRouteWithLegs).toHaveBeenCalledTimes(3) + expect(result.current.days[0].legs).toEqual([undefined, undefined]) + }) + it('FE-ROADTRIP-ROUTES-009: renaming a place does not re-ask the router', async () => { const days = [day(1, 1)] const stops: StopSpec[] = [{ id: 1, at: HAMBURG }, { id: 2, at: BERLIN }] diff --git a/client/src/components/Roadtrip/useRoadtripRoutes.ts b/client/src/components/Roadtrip/useRoadtripRoutes.ts index 62dcd5d0f..2637f3d94 100644 --- a/client/src/components/Roadtrip/useRoadtripRoutes.ts +++ b/client/src/components/Roadtrip/useRoadtripRoutes.ts @@ -278,6 +278,110 @@ export function useRoadtripRoutes( const collectedMisses: Record = {} const tasks: (() => Promise)[] = [] + /** + * Ask for one run, and when the router refuses the whole of it, ask for its legs + * one at a time instead. + * + * A run travels as a single request because the answer carries a leg per waypoint + * pair, which is exactly what the rail wants, and one request is one slot of a + * host that allows about one a second. What that thrift costs is that a refusal is + * never local: OSRM answers `400 NoRoute` for the entire chain when one stop in the + * MIDDLE of it sits where it may not turn around, so a day lost every leg it had + * over a single cave car park while each of its pairs routed perfectly alone. + * `uTurnParam` in RouteCalculator removes the usual cause; this removes the SHAPE of + * the failure, so a stop that really cannot be reached costs its own two legs and + * not the day around them. + * + * The split is enqueued, not awaited, so the pairs queue up behind everything else + * and keep the same spacing as any other request: a day falling back must not turn + * into a burst at the one host that refused it. + */ + const enqueueRun = (day: PlanDay, dayLegs: Record, run: RoadtripStop[], mode: string): void => { + /** The same stops, as one request per pair. A pair has nothing left to split. */ + const splitIntoPairs = (): void => { + if (run.length <= 2) return + for (let i = 0; i < run.length - 1; i++) enqueueRun(day, dayLegs, [run[i], run[i + 1]], mode) + } + tasks.push(async () => { + // Waypoints are the stops with this day's vias threaded in between them, so the + // router draws the road the traveller picked. `stopAt` remembers which waypoint + // each stop became, because the answer has a leg per waypoint PAIR and the rail + // wants one leg per stop pair. + const waypoints: { lat: number; lng: number }[] = [] + const stopAt: number[] = [] + run.forEach((stop, i) => { + stopAt.push(waypoints.length) + waypoints.push({ lat: stop.lat, lng: stop.lng }) + if (i === run.length - 1) return + // A via is filed against the day it was dropped on and the position it sits + // after IN THAT DAY. Since a chain can hold stops from an earlier day (see + // `nightSpill.ts`), the lookup has to be the stop's own day and its own + // index — this used to be the position within the chain, so on any day that + // received a night drive every via matched nothing and quietly stopped + // shaping the road. + ;(viasByDay[stop.ownerDayId ?? day.dayId] ?? []) + .filter(v => v.after_order_index === (stop.ownerIndex ?? i)) + .sort((a, b) => a.sequence - b.sequence) + .forEach(v => waypoints.push({ lat: v.lat, lng: v.lng })) + }) + + for (let attempt = 0; attempt <= RETRY_DELAYS_MS.length; attempt++) { + if (controller.signal.aborted) return + try { + const r = await calculateRouteWithLegs( + waypoints, + { signal: controller.signal, profile: mode, tripId: tripId ?? null, dayId: day.dayId, avoid }, + ) + // Where each stop ended up. stopAt[i] is that stop's waypoint index, so the + // vias threaded in between are skipped: a via is a shape handle, not a + // destination, and a dashed spur hanging off one reads as a fault. + if (r.snapped) { + const daySnaps = (collectedSnaps[day.dayId] ??= {}) + run.forEach((stop, i) => { + const s = r.snapped?.[stopAt[i]] + if (s) daySnaps[stopKey(stop)] = s + }) + } + // What was asked for against what the road turned out to be. Only + // the second engine reports it, and only when it answered: an OSRM + // fallback leaves `avoidance` absent, which is the honest reading + // of "the weighting never happened". Collected here because the + // rail plans in the browser and never sees the server's own copy + // of this field — without it the "not honoured" badge could not + // appear at all, and a motorway-free drive that is not one read as + // if the setting had held. + if (r.avoidance) { + const missed = r.avoidance.asked.filter(cls => !r.avoidance!.achieved.includes(cls)) + if (missed.length) { + collectedMisses[day.dayId] = [...new Set([...(collectedMisses[day.dayId] ?? []), ...missed])] + } + } + Object.assign(dayLegs, foldRouteRun(run, stopAt, r, mode)) + return + } catch (err) { + if (controller.signal.aborted) return + const refusal = err instanceof RoutingRefusedError ? err : null + // Almost always a rate limit on the shared routing host, or a connection + // that dropped: back off and try again. A refusal the host MEANT is the + // one thing not worth repeating: it objects to these coordinates, and + // asking twice more only spends five seconds earning the same 400. + const delay = refusal && !refusal.isRateLimit ? undefined : RETRY_DELAYS_MS[attempt] + if (delay !== undefined) { + // When the host says how long to wait, waiting less is just a second refusal. + await sleep(Math.max(delay, (refusal?.isRateLimit ? refusal.retryAfterMs : null) ?? 0), controller.signal) + continue + } + // Nothing left to try. A run of three or more stops asks again one pair at + // a time, so a stop the router will not route THROUGH costs its own two + // legs instead of every leg of the day around it. Never after a rate limit: + // the answer to a host asking for less traffic is not four more requests. + if (!refusal?.isRateLimit) splitIntoPairs() + return + } + } + }) + } + for (const day of plan) { const dayLegs: Record = {} collected[day.dayId] = dayLegs @@ -290,77 +394,7 @@ export function useRoadtripRoutes( dfMode, )) - - for (const { stops: run, mode } of runs) { - tasks.push(async () => { - // Waypoints are the stops with this day's vias threaded in between them, so the - // router draws the road the traveller picked. `stopAt` remembers which waypoint - // each stop became, because the answer has a leg per waypoint PAIR and the rail - // wants one leg per stop pair. - const waypoints: { lat: number; lng: number }[] = [] - const stopAt: number[] = [] - run.forEach((stop, i) => { - stopAt.push(waypoints.length) - waypoints.push({ lat: stop.lat, lng: stop.lng }) - if (i === run.length - 1) return - // A via is filed against the day it was dropped on and the position it sits - // after IN THAT DAY. Since a chain can hold stops from an earlier day (see - // `nightSpill.ts`), the lookup has to be the stop's own day and its own - // index — this used to be the position within the chain, so on any day that - // received a night drive every via matched nothing and quietly stopped - // shaping the road. - ;(viasByDay[stop.ownerDayId ?? day.dayId] ?? []) - .filter(v => v.after_order_index === (stop.ownerIndex ?? i)) - .sort((a, b) => a.sequence - b.sequence) - .forEach(v => waypoints.push({ lat: v.lat, lng: v.lng })) - }) - - for (let attempt = 0; attempt <= RETRY_DELAYS_MS.length; attempt++) { - if (controller.signal.aborted) return - try { - const r = await calculateRouteWithLegs( - waypoints, - { signal: controller.signal, profile: mode, tripId: tripId ?? null, dayId: day.dayId, avoid }, - ) - // Where each stop ended up. stopAt[i] is that stop's waypoint index, so the - // vias threaded in between are skipped: a via is a shape handle, not a - // destination, and a dashed spur hanging off one reads as a fault. - if (r.snapped) { - const daySnaps = (collectedSnaps[day.dayId] ??= {}) - run.forEach((stop, i) => { - const s = r.snapped?.[stopAt[i]] - if (s) daySnaps[stopKey(stop)] = s - }) - } - // What was asked for against what the road turned out to be. Only - // the second engine reports it, and only when it answered: an OSRM - // fallback leaves `avoidance` absent, which is the honest reading - // of "the weighting never happened". Collected here because the - // rail plans in the browser and never sees the server's own copy - // of this field — without it the "not honoured" badge could not - // appear at all, and a motorway-free drive that is not one read as - // if the setting had held. - if (r.avoidance) { - const missed = r.avoidance.asked.filter(cls => !r.avoidance!.achieved.includes(cls)) - if (missed.length) { - collectedMisses[day.dayId] = [...new Set([...(collectedMisses[day.dayId] ?? []), ...missed])] - } - } - Object.assign(dayLegs, foldRouteRun(run, stopAt, r, mode)) - return - } catch (err) { - // Almost always a rate limit on the shared routing host. Back off and try - // again; a run that still won't route (island hop, dead router) simply stays - // blank, and the totals say so by being partial rather than wrong. - const delay = RETRY_DELAYS_MS[attempt] - if (delay === undefined) return - // When the host says how long to wait, waiting less is just a second refusal. - const asked = err instanceof RoutingRefusedError && err.isRateLimit ? err.retryAfterMs : null - await sleep(Math.max(delay, asked ?? 0), controller.signal) - } - } - }) - } + for (const { stops: run, mode } of runs) enqueueRun(day, dayLegs, run, mode) } // One at a time, spaced out. The day sidebar can afford a small pool because it @@ -402,7 +436,12 @@ export function useRoadtripRoutes( const allLegs = useMemo(() => { const out: Record = {} for (const day of plan) Object.assign(out, legsByDay[day.dayId] ?? {}) - return { ...out, ...seamLegs } + // Seams first, this day's own runs over the top. `legsByDay` is replaced whole on + // every routing round while `seamLegs` only ever grows, so a pair that used to sit on + // a day boundary and now sits inside one day keeps an answer nobody re-asks for, and + // that older answer was shaped by the vias of a stop that has since moved. Where both + // exist the day run is the newer of the two, so it is the one to believe. + return { ...seamLegs, ...out } }, [plan, legsByDay, seamLegs]) // Which date each stop is actually reached on. Nothing is written to make it so; see