fix(area): extraction never finished on a plaza with two nodes in one place - #7695
Conversation
… place Dijkstra::run could leave a cycle in the predecessor array, and run_dijkstra walks that array with no bound, so extraction ran forever: no output, no error, nothing to look at. Ile-de-France did not finish in 33 minutes. With this it takes 1 minute 57 seconds. The tie-break that keeps the choice of predecessor deterministic when two paths are equally long had no notion of a settled vertex, so it would rewrite the predecessor of a vertex whose distance was decided long ago. That needs an edge of no length between two vertices the same distance from the source, which is ordinary input: plaza rings routinely carry two nodes mapped at the same place, and the ring edge between them weighs zero. Both vertices then settle together and each becomes a better predecessor for the other than the source is, so each ends up pointing at the other. Fixed at the source with a settled set. That also makes the result acyclic by construction rather than by luck: a vertex only ever points at one that settled before it did, so following predecessors strictly decreases settle time and has to reach the root. The tie-break still runs, and still picks the lower vertex, so which of two equal paths wins is unchanged. run_dijkstra's walk is bounded as well. A tree of n vertices has no path longer than n edges, and that loop runs over every vertex of every area in the input, where the cost of being wrong is an extraction that never ends. It now says so and moves on. Found by bisecting: this has been present since areas were first meshed in #7161. One 2 MB tile of Ile-de-France produced 43 of these cycles.
There was a problem hiding this comment.
Pull request overview
Fixes a non-terminating osrm-extract failure mode in area meshing by preventing cycles in Dijkstra’s predecessor array (triggered by zero-length edges between co-located plaza ring nodes), and adds a regression test to lock in the acyclic-predecessor property that downstream path recovery relies on.
Changes:
- Update extractor-area Dijkstra to track a settled set so popped vertices are never relaxed into again, preventing predecessor cycles.
- Add a bounded predecessor-walk (with warning) when collecting shortest-path-tree edges during area meshing.
- Add a unit test that reproduces the zero-length edge / equal-distance scenario and asserts predecessors remain acyclic and tie-break behavior stays stable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| include/extractor/area/dijkstra.hpp | Adds a settled set to Dijkstra to prevent predecessor rewrites into already-finalized vertices (eliminates cycles). |
| src/extractor/area/area_mesher.cpp | Bounds predecessor walking during shortest-path-tree collection and logs a warning if a cycle is detected. |
| unit_tests/extractor/area/dijkstra.cpp | Adds a regression test ensuring predecessor chains terminate at the source in the zero-weight edge scenario. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (step > d.num_vertices()) | ||
| { | ||
| util::Log(logWARNING) | ||
| << "Shortest-path tree has a cycle at node " << d.get_vertex(v).ref() | ||
| << ", giving up on this entry point."; |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7695 +/- ##
==========================================
- Coverage 94.75% 94.73% -0.02%
==========================================
Files 519 519
Lines 41582 41610 +28
==========================================
+ Hits 39402 39421 +19
- Misses 2180 2189 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
The two codecov failures are on the bounded walk in Verified it does work, by building with only the guard and the defect left in. On a 2 MB tile of Ile-de-France it fires 43 times and extraction completes; with both changes it fires zero times. Happy to drop the guard if the preference is to trust the invariant, but a loop over every vertex of every area whose failure mode is an extraction that never ends and never says why seems worth the four lines. |
Issue
No issue. Found while trying to build Ile-de-France with
profiles/foot_area.lua.The symptom
osrm-extractnever finishes. No output file, no error, no warning. Ile-de-France ran for33 minutes at full CPU with flat memory and had written nothing but the timestamp file.
With this change the same extract takes 1 minute 57 seconds.
It is not size-dependent. A 2 MB tile around Mantes-la-Jolie, containing four pedestrian
areas of which the largest is 87 points, hangs just as reliably.
The defect
Dijkstra::runcan leave a cycle in the predecessor array, andrun_dijkstrawalksthat array with no bound:
while (v != u && v != predecessors.at(v)) v = predecessors.at(v);Neither exit condition is ever reached when two entries point at each other.
The cycle comes from the tie-break that keeps the choice of predecessor deterministic when
two paths are equally long. It has no notion of a settled vertex, so it will rewrite the
predecessor of a vertex whose distance was finalised long ago.
Producing one needs an edge of no length between two vertices the same distance from the
source. That sounds contrived and is not: plaza rings routinely carry two nodes mapped at
the same location, and the ring edge between them weighs zero. Both vertices then settle at
the same distance, and each becomes a better predecessor for the other than the source is,
so each ends up pointing at the other.
That is also why no test caught it. Every existing case uses positive edge weights.
The fix
A settled set, so a vertex that has been popped is never relaxed into again. Beyond
being what Dijkstra is supposed to do, it makes the output acyclic by construction rather
than by luck: a vertex only ever points at one that settled strictly before it did, so
following predecessors strictly decreases settle time and has to terminate at the root.
The tie-break still runs and still prefers the lower-numbered vertex, so which of two
equally short paths wins is unchanged. The three existing Dijkstra tests pass untouched, and
the full cucumber suite is unmoved.
The walk in
run_dijkstrais bounded as well, with a warning. A tree of n vertices hasno path longer than n edges. That loop runs over every vertex of every area in the input,
and the cost of it being wrong is an extraction that produces nothing and says nothing, so
it should not be trusted even though it is now correct.
Both halves checked separately
2 MB tile.
rather than papered over.
How old is this
Bisected. The first bad commit is
506fac3c7, "feat(area): route across pedestrian areas bymeshing them" (#7161), which is where area meshing was introduced. It has been there since
the feature landed, and it is not a regression from #7691 or #7692.
Testing
New case
area_dijkstra_leaves_no_cycle_in_the_predecessors, which states the propertyevery caller depends on and none of them can check: following the predecessors from
anywhere reaches the source. It fails without the fix. It also pins the distances and the
tie-break winner, so a future change cannot "fix" the cycle by throwing the determinism away.
All fourteen unit suites pass. Full cucumber: 1478 scenarios, 1463 passed, 15 skipped, 0
failed, the same as before the change.
End to end on Ile-de-France with
foot_area.lua: extract 1m57s, partition 47s, customize18s, zero cycle warnings, and
osrm-routedthen answers foot routes across Place de laRépublique, Place de la Bastille, Place de la Madeleine, the Notre-Dame parvis, Trocadéro,
Place de la Sorbonne and Place du Châtelet.
Was this change primarily generated using an AI tool? Yes.
🤖 Claude Code, Claude Opus 5
Tasklist
Requirements / Relations
Independent of #7693 and #7694. This one should go first: without it no profile that calls
area_manager:initcan be used on a real extract.