[hir-al-14]

Summary

• Adds traffic-aware forecasting after the weather-aware route solve, so the backend can estimate traffic delay before returning the final optimized route.
• Adds forecast metadata for traffic impact, delay, provider, thresholds, and whether traffic caused a second route solve.

Motivation

• Weather alone does not explain route timing well enough for real driver planning.
• This adds the next basic piece of the forecast pipeline: use the optimized route legs, check expected traffic delay, and only rerun the route when the delay is large enough to matter.

Changes

• Backend:

  • Added traffic forecast options from env, including enable flag, Google Maps base URL/API key, and reoptimization thresholds.
  • Added Google Maps traffic request helpers for route legs from the VROOM result.
  • Added traffic delay calculation and threshold logic.
  • Wired traffic after the weather-aware VROOM solve.
  • Reruns VROOM only when traffic delay crosses the configured threshold.
  • Adds traffic forecast metadata to the optimize response.
  • Fixed traffic leg departure timing so Google traffic requests use the correct route clock.

• Infrastructure:

  • Passed traffic forecast env settings through Docker compose config.

Validation

Frontend

• npm --prefix app/ui run lint
• npm --prefix app/ui run format:check
• npm --prefix app/ui run typecheck
• npm --prefix app/ui run test
• npm --prefix app/ui run build
• npm --prefix app/mobile run lint
• npm --prefix app/mobile run typecheck

Backend

• cmake --preset dev
• .github/scripts/check-backend-static.sh build/dev
• cmake --build --preset conan-release --parallel
• git diff --check
• docker compose -f deploy/compose/docker-compose.arm64.yml --env-file deploy/env/http-server.arm64.env config
• cmake --build --preset dev --parallel
• ctest --preset dev --output-on-failure --no-tests=error -LE 'e2e|docker'

Risk

• Google Maps traffic calls depend on a valid API key and network access.
• The current reroute model adds traffic delay into service time as a simple version. It does not build a traffic-aware matrix.

Rollout and Recovery

• Roll out by enabling DELIVERYOPTIMIZER_TRAFFIC_FORECAST_ENABLED=1 and setting GOOGLE_MAPS_API_KEY.
• The route solve falls back to the existing weather/VROOM behavior. (DELIVERYOPTIMIZER_TRAFFIC_FORECAST_ENABLED to 0)

High-Signal PR Checklist

• The summary states the user-visible or operational outcome, not just file names.
• The motivation explains why this change is needed now.
• The change list separates frontend, backend, infrastructure, and documentation work where applicable.
• Validation includes exact commands run, relevant output, and any checks intentionally skipped.
• Risks, migrations, feature flags, and rollback steps are called out when relevant.
• Screenshots or request/response examples are included for UI and API behavior changes.

[markboenigk]

Hey Hiral — good progression from #180. The traffic layer is well-structured: the feature-flag defaults are safe, the env variable naming is consistent, the threshold logic mirrors the weather path cleanly, and the new unit tests cover the traffic path well (ReadsTrafficLegsFromVroomSteps, ReadsGoogleTrafficDelay, BuildsGoogleTrafficPath are all good additions). The OSRM entrypoint CRLF fix and Windows portability guards are also nice housekeeping.

A few things to work through before merge:

Must-fix

1. ReadRouteTraffic serially blocks the event loop — new, critical. FetchTrafficDelay is called per leg from the event loop completion lambda, and each call blocks via future.wait_for(5s). A 10-stop route can stall the server for up to 50 seconds. This is the same class of bug as the weather future.wait_for() — it just compounds by stop count here. All three blocking callers (OpenWeather fetch, weather reoptimize runner->Run(), and now the traffic leg loop) need to move off the event loop thread — worker thread pool, drogon::async_run, or a chained async callback are all valid paths.

2. ResolveTrafficForecastOptionsFromEnv() called per job in WorkerLoop. Same issue as ResolveWeatherForecastOptionsFromEnv() from feat: use route duration accurate for weather-aware optimization #180 — both are now resolved inside the per-job loop in optimization_job_runtime.cpp. Both should be resolved once at construction and stored on the runtime.

3. Thunder double-count in DelayFromHourlyForecast — carryover from feat: use route duration accurate for weather-aware optimization #180, not fixed here. Thunderstorm hours (weather id 200–299) also include a rain object in the OpenWeather payload, so the function adds 240 + 90 = 330s instead of 240s.

Worth fixing

4. ReadLegDeparture heuristic needs a comment. The check offset >= route_start_seconds - 24h is how the code distinguishes "VROOM produced absolute Unix timestamps" from "VROOM produced relative seconds." That's clever, but nothing tells the next reader it's intentional. A single comment line would prevent someone from "simplifying" it away. A test for the relative-offset path would also help.

5. #undef GetJob needs an explanation. The undef in two files is clearly a Windows SDK macro conflict, but without a comment it reads as magic. One line (// Windows SDK defines GetJob as a macro; undef before including jsoncpp) is enough.

6. Traffic delay spread uniformly across stops. Dividing total delay equally per job is a known simplification, but for routes with high variance in leg length it gives VROOM a misleading service-time vector. Even a rough proportional split by VROOM leg duration would be more accurate. Worth flagging as a known limitation in a comment if not changing it now.

7. Redundant response fields — carryover from feat: use route duration accurate for weather-aware optimization #180: baseline_duration_seconds == baseline_route_duration_seconds always, and weather_adjusted_duration_seconds == predicted_duration_seconds always. These will confuse API consumers.

Frontend carryovers from #180

Location	Issue
StopCard.tsx	"Call" button uses onClick={onNavigate} — should open tel:${stop.phoneNumber}
StopCard.tsx	Hardcoded "Deliver between 4:00pm - 5:00pm" — should use the stop's actual time window
upload-route/page.tsx	JSX still uses className="ur-root" / "ur-content" but CSS was renamed to .upload-root / .upload-content — element is unstyled
upload-route/page.tsx	const [isProcessing] = useState(false) — setter removed, state is dead; handleContinue has no error handling for a rejected file.text()
summary/page.tsx	Warning button has no onClick
summary/page.tsx	setTimeout(0) workaround doesn't reliably fix the read-after-navigate race
manifest.ts	purpose: "maskable" on a full-bleed SVG — content reaches the canvas edges and will be clipped on Android

The event-loop threading fix is the one I'd want to see before this merges. Everything else can be sequenced, but the leg-loop blocking is a regression in server availability under real traffic conditions. Happy to talk through the async approach if that's helpful.

[markboenigk]

Good iteration — all three must-fixes from the last round are in and clean. The thunder double-count fix is especially nice, the else if reads clearly and the test locks it down. The threading refactor in FinishWithTraffic and the constructor-time env resolution are both right.

A few new things surfaced:

Worth fixing before merge

1. Job ID / array-position mismatch in BuildTrafficAdjustedVroomInput (forecast_optimizer.cpp:955–958)
   ReadJobTravelSeconds fills travel_seconds[raw_job_id - 1], so delay allocation is indexed by VROOM job ID. BuildTrafficAdjustedVroomInput then writes traffic_delays[index] into payload["jobs"][index] by array position. These align only if input job IDs are 1-based and sequential. If they're not (non-contiguous IDs, or a partial job set), stop A gets stop B's traffic penalty — VROOM sees a wrong service-time vector and the reopt solve is silently wrong. Worth adding an ID→position map or documenting the assumption with an assertion.

2. useSyncExternalStore no-op subscribe on summary/page.tsx (page.tsx:1110–1114)
   Replacing setTimeout(0) with useSyncExternalStore is the right idea, but subscribeToRouteStore = () => () => undefined means the component can't react if localStorage is written after mount. If the active-route page hasn't flushed before Summary mounts, readSavedRoute() returns null, the redirect fires, and the driver loses the summary screen with no recovery path. A beforeunload flush on the active-route side — or a storage event subscription here — would close this properly.

Lower priority

3. FetchTrafficDelay timeout contract (forecast_optimizer.cpp:757–764)
   The promise wait_for(5s) path when not_ready is safe. The question is whether drogon guarantees the response callback is always called on timeout (so the promise is always resolved). If drogon can silently drop the callback, the future resolves only after the destructor — which is fine here since wait_for guards it, but worth verifying against drogon's docs before a production key is set.

Frontend carryovers (same as last time, sequencing is fine):
StopCard.tsx "Call" still routes via onNavigate instead of tel:, upload-route/page.tsx dead isProcessing setter + no error handling on file.text().

The job ID indexing one is the new must-look-at — everything else can be sequenced or documented. Happy to talk through the ID→position fix if useful.

[markboenigk]

Code review (medium effort) — 2 blocking perf/correctness issues, 1 cleanup item

CI is green and the PR is mergeable. The traffic delay math, weighted service-time distribution, and leg-departure heuristics are well unit-tested, and the frontend changes (phone-call link, driver-assist summary auto-refresh, dead warning button removal) are clean, self-contained improvements. However, two issues in the traffic-forecast pipeline should be addressed before merging:

Blocking:

1. app/api/src/forecast_optimizer.cpp:641 — ReadRouteTraffic fetches Google Maps traffic delay for every route leg sequentially and synchronously, each call blocking up to kGoogleMapsTimeoutSeconds+1 (5s). A route with 20+ stops produces 19+ legs; if Google Maps is slow/unreachable, each FetchTrafficDelay call blocks ~5s before falling back, so the traffic step alone can add 1-2+ minutes of latency to every optimize response. Suggest firing the per-leg calls concurrently (small thread pool) or switching to a batched Distance Matrix request.

2. app/api/src/endpoints/deliveries_optimize_endpoint.cpp:143 — FinishWithTraffic spawns a new detached std::thread for every completed weather solve, unconditionally (even when DELIVERYOPTIMIZER_TRAFFIC_FORECAST_ENABLED is unset, the default), with no bound tied to the existing SolveCoordinator admission control. Under load this creates OS threads outside the coordinator's queueing/admission limits — combined with #1, many concurrent large routes can pile up long-lived native threads that the coordinator was designed to prevent.

Non-blocking cleanup:

3. app/api/src/forecast_optimizer.cpp:539 — FetchTrafficDelay re-implements the same HttpClient + promise/future + timeout pattern already used by FetchOpenWeatherDelayEstimate (lines 205-267) almost verbatim. Worth sharing a helper so future fixes to the blocking-HTTP-with-timeout pattern don't have to be made twice.

[markboenigk]

Review: Traffic-aware route forecast (#181)

Nice feature. A few blocking issues around fallback behavior and a client-side render loop, plus some cleanups.

Blocking

1. Infinite render loop on the driver summary screen — app/ui/src/app/driver_assist/summary/page.tsx:40
useSyncExternalStore is called with an unstable getSnapshot: readSavedRoute() runs JSON.parse + parsePersistedRouteState on every call and returns a fresh object reference. Previously subscribeToRouteStore was a guaranteed no-op so this was harmless, but this PR wires it to real ROUTE_STORE_EVENT / storage listeners and persistRoute now dispatches ROUTE_STORE_EVENT on every save. With a route in localStorage, React's post-commit checkIfSnapshotChanged always sees a new reference → forceStoreRerender → new snapshot → "Maximum update depth exceeded", crashing the summary screen and every stop completion in the driver tab. Memoize the snapshot (cache the parsed object, return the same reference until the serialized value changes).

2. Failed traffic re-optimization discards an already-good route — app/api/src/optimization_job_runtime.cpp:178
final_result holds a valid (possibly weather-adjusted) route. When traffic.should_reoptimize, the result of runner_->Run(BuildTrafficAdjustedVroomInput(...)) is assigned straight into final_result with no fallback. If the vroom subprocess times out or errors on that extra rerun, final_result becomes empty/failed and the job is marked kFailed despite a usable route being in hand. FinishWithTraffic in deliveries_optimize_endpoint.cpp:138-144 has the same no-fallback pattern for the sync endpoint. Keep the prior result unless the rerun succeeds.

3. Out-of-range departure_time silently disables traffic for the whole route — app/api/src/forecast_optimizer.cpp:202
ReadLegDeparture can return a departure_time far in the future or in the past:

• When route_start_time is absent, line 202 returns now() + offset. If vroom emitted absolute Unix-timestamp arrivals (input had absolute time windows), offset is ~1.7e9s and the departure is decades out.
• When route_start_time is in the past (async job processed after the route start, or route already dispatched), line 199 returns a past departure.

Google Distance Matrix requires departure_time to be now-or-future, so those elements come back status != "OK", ReadTrafficDelay returns nullopt, and every leg is treated as no-traffic — the feature no-ops with nothing surfaced. Clamp to >= now() and guard against implausibly large offsets.

Non-blocking

4. Duplicated traffic post-processing orchestration — app/api/src/endpoints/deliveries_optimize_endpoint.cpp:98
The ReadRouteTraffic → EstimateTrafficImpact → AddTrafficForecast → conditional reoptimize sequence is duplicated in deliveries_optimize_endpoint.cpp:98-155 and optimization_job_runtime.cpp:170-181, and they already diverge (the endpoint routes the rerun through the coordinator + admission control; the job runtime calls runner_->Run directly). Worth extracting to one helper so threshold semantics and fallback handling stay in sync.

5. Misleading workers.clear() — app/api/src/forecast_optimizer.cpp:616
workers.clear() right after spawning the traffic-worker jthreads reads as "cancel in-flight requests" but it's a blocking join — the worker lambdas take no stop_token and never check one, so this just waits for every Google Maps HTTP call to finish. Let workers fall out of scope, or add a comment / explicit join, to make the intent clear.

6. Call control lost button semantics — app/ui/src/app/driver_assist/components/StopCard.tsx:117
Changed from <button> to <a href="tel:"> but still uses styles.actionButton, which was authored for a <button>. Without role="button" and a text-decoration/color reset, the Call link can render underlined / visited-link-colored and is exposed to assistive tech as a link rather than matching the adjacent Navigate/Report buttons.

[markboenigk]

The traffic forecast work looks well-structured — the largest-remainder distribution, the PreferSuccessfulRerun handling on both paths, and the index alignment across BuildTrafficAdjustedVroomInput all hold up under a close read. Two things block merge, and one is about the shape of the PR rather than the code.

Blocking: please split this PR

The body describes traffic forecasting, but the diff is 51 files / 3,315 insertions, and 8 of those files are driver_assist/ and upload-route/ UI work that isn't mentioned.

Those UI commits are rebased copies of the ones in #174 — same headlines, different SHAs (beaa3fa "handle CSV route uploads in Driver Assist", 8d5247d "show invalid route upload errors on upload page"). But this branch is missing all three of #174's latest round of fixes:

unify efficient route import parsing    MISSING
validate and reuse uploaded routes      MISSING
explain deferred hydration state        MISSING

and it carries two UI commits #174 doesn't have (f8ce573, a11901a). So the same files are evolving down two branches at once. Whichever merges second conflicts, and a careless resolution silently reverts fixes that #174's review rounds already paid for.

Ask: drop the UI commits from this branch and let #174 own that work. I'll review the API half here. I can't usefully review the driver_assist changes in two places simultaneously, and neither can you maintain them.

Blocking: the sync path blocks completion workers on Google

ReadRouteTraffic (forecast_optimizer.cpp:592) issues one Distance Matrix request per route leg — roughly stops + vehicles — kMaxConcurrentTrafficRequests = 4 at a time, each blocking up to kGoogleMapsTimeoutSeconds = 4 + 1 seconds in FetchJsonWithTimeout, then joins every worker on the calling thread.

On the sync path that runs inside a SolveCoordinator completion callback, and there are only max_concurrency completion workers (ResolveCompletionWorkerCount, solve_coordinator.cpp:38). A 60-stop route with Google slow or unreachable holds one worker for ~75 s. max_concurrency such calls stall every pending response, including error responses that never touch traffic at all.

The fix template is already in this file. deliveries_optimize_endpoint.cpp:273-285 builds a sync_weather_options copy with .openweather_api_key = {} and the comment:

synchronous completion workers must not block on remote weather.

Traffic has no equivalent — line 154 passes *context->traffic_options through intact. Mirror the weather guard: build a sync-path traffic options copy with the API key and base URL cleared, so IsGoogleMapsConfigured short-circuits and only the async path talks to Google.

Acceptance: an integration test in the shape of the existing deliveries_optimize_*_test.sh files showing a sync solve completes promptly with traffic enabled and an unreachable Google endpoint.

Separately worth confirming before rollout: at one Distance Matrix element per leg, each optimize call bills N+1 elements. Is that costed?

Blocking: an exception in the worker thread aborts the process

The std::jthread lambda at forecast_optimizer.cpp:611 has no try/catch. FetchTrafficDelay → FetchJsonWithTimeout ends in future.get(), and wait_for reports ready for a broken promise as well as a satisfied one — so if drogon drops a sendRequest callback without invoking it (client torn down mid-flight, e.g. shutdown), get() throws std::future_error. An exception escaping a thread function is std::terminate() — the whole API process, not a degraded forecast. Same exposure for std::bad_alloc on a large response.

Before this PR the equivalent code ran on the request thread, where it could at least unwind.

Acceptance: catch (...) inside the worker lambda, recording the leg as unavailable. The traffic feature degrading to "unavailable" is always preferable to the process dying.

Should fix before merge

forecast_optimizer.cpp:208 — wrong day's traffic, reported as authoritative. When route_start_time is nullopt, ReadLegDeparture treats any offset over 7 days as implausible and falls back to now. But vehicle time_window and job time_windows are independently optional (optimize_request.cpp:271 / :350), so a request with absolute job windows and no vehicle window is valid — VROOM emits absolute epoch arrival values while ReadRouteStartTime returns nullopt. A route planned for tomorrow 06:00 gets priced against this afternoon's traffic and the response still says provider: "google_maps". If the start time can't be established, the honest output is "traffic unavailable", not a confident wrong number.

forecast_optimizer.cpp:211 — BuildEvenTrafficDelays over-applies by up to a full job's worth per job. Every job gets ceil(total_delay / job_count), so the padding actually injected is job_count * ceil(...). 100 jobs and 150 s measured → 200 s applied, 33 % over. BuildWeightedTrafficDelays directly below distributes the total exactly via largest-remainder; the even path should use the same approach. This is the fallback for the degraded case (VROOM output with no arrival fields), which is where accuracy matters most.

forecast_optimizer.cpp:815 — a misconfigured rollout looks healthy. AddTrafficForecast sets status: "evaluated" whenever options.enabled, and provider is impact.source, which is "" when nothing was fetched. Set DELIVERYOPTIMIZER_TRAFFIC_FORECAST_ENABLED=1 with an empty or rejected GOOGLE_MAPS_API_KEY and the API returns {"status":"evaluated","provider":"","traffic_delay_seconds":0} — indistinguishable from "we checked, traffic is fine". Please emit a distinct status (unavailable / not_configured) when no provider answered.

Not blocking

• summary/page.tsx:47 — redirect effect deps narrowed to [router] while route still comes from useSyncExternalStore, so a cleared route in another tab renders an empty summary with a live Export button instead of redirecting.
• storage.ts:70 — persistRoute caches the caller's object rather than parsePersistedRouteState(...)'s output, so the in-memory snapshot can differ from what the same route deserializes to after reload.

Both are in the UI half; they travel with #174 if you split as asked.

Checked and clean — no need to revisit

• Index alignment in BuildTrafficAdjustedVroomInput: ReadPayloadJobIds, BuildWeightedTrafficDelays, BuildEvenTrafficDelays all return exactly payload["jobs"].size() entries on every branch.
• Job-id matching: BuildVroomInputText emits 1-based ids, so the raw_job_id == 0 skip can't drop a real job.
• EstimateTrafficImpact's max(absolute, percent) threshold matches EstimateWeatherImpact.
• PreferSuccessfulRerun applied on both sync and async reruns; context->traffic_baseline always engaged before dereference; responded guard prevents double-response on the rejection branch.
• Weather delay is not double-counted — BuildWeatherAdjustedVroomInputText no-ops when !should_reoptimize.
• parseCsvToRows filters all-empty rows; packageCount ?? 1 and lat ?? 0 preserve explicit zeros as intended.

What I'd like back

The split first — once the UI commits are gone I'll re-read the API half in one pass. Then the sync-path guard and the catch (...), with the integration test. The three "should fix" items can land in the same push; I don't need a separate round for them.

[markboenigk]

This branch is an ancestor of both #212 and #213 — everything here is already included further down that chain. The traffic-forecast bugs that originate in this PR (weighted-delay leg mismatch, ceil-rounding over-injection in the fallback path, unbounded per-call thread spawn, stale weather-impact reuse across reruns) are called out on #213, which is the PR to actually fix and merge. Recommend closing this one rather than merging it independently.