fix(area): extraction never finished on a plaza with two nodes in one place

include/extractor/area/dijkstra.hpp

@@ -140,13 +140,33 @@ template <class vertex_t> class Dijkstra
         distances[s] = 0;
         pq.insert(s);
 
+        // Which vertices have been popped, and so have their final distance.
+        //
+        // Without this the tie-break below can rewrite the predecessor of a vertex that
+        // was settled long ago.  Two adjacent vertices the same distance from the source
+        // then end up pointing at each other, and the predecessor array, which every
+        // caller walks to recover a path, has a cycle in it.
+        std::vector<bool> settled(vertices.size(), false);
+
         while (!pq.empty())
         {
             size_t u = pq.pop();
+            if (settled[u])
+            {
+                continue;
+            }
+            settled[u] = true;
             double dist_u = distances[u];
             for (Edge e : adj[u])
             {
                 size_t v = e.other;
+                if (settled[v])
+                {
+                    // Its distance is final and so is the way it was reached.  This is
+                    // also what keeps the predecessors acyclic: a vertex only ever
+                    // points at one that settled before it did.
+                    continue;
+                }
                 const double candidate = dist_u + e.weight;
                 if (significantly_shorter(candidate, distances[v]))
                 {
@@ -157,6 +177,8 @@ template <class vertex_t> class Dijkstra
                 else if (approximately_equal(candidate, distances[v]) &&
                          vertices[u] < vertices[predecessors[v]])
                 {
+                    // Equally short, so the choice is free and is made the same way on
+                    // every platform and in every run: the smaller vertex wins.
                     predecessors[v] = u;
                     pq.insert_or_decrease(v);
                 }

src/extractor/area/area_mesher.cpp

@@ -504,11 +504,22 @@ std::set<OsmiumSegment> AreaMesher::run_dijkstra(const OsmiumPolygon &poly,
         for (index_t target = 0; target < d.num_vertices(); ++target)
         {
             index_t v = target;
-            while (v != u && v != predecessors.at(v))
+            // A tree of n vertices has no path longer than n edges, so a walk that has
+            // not reached the root by then is going round a cycle.  Dijkstra does not
+            // produce one, but this is a loop over every vertex of every area in the
+            // input, and the cost of it being wrong is that extraction never finishes:
+            // no output, no error, nothing to look at.  So it is bounded here and says
+            // so, rather than being trusted.
+            for (index_t step = 0; v != u && v != predecessors.at(v); ++step)
             {
+                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.";
+                    break;
+                }
                 auto s = OsmiumSegment(d.get_vertex(v), d.get_vertex(predecessors.at(v)));
-                util::Log(logDEBUG)
-                    << "    Collecting: " << s.first.ref() << " -> " << s.second.ref();
                 result.emplace(s);
                 v = predecessors.at(v);
             }

unit_tests/extractor/area/dijkstra.cpp

@@ -132,4 +132,53 @@ BOOST_AUTO_TEST_CASE(area_dijkstra_equal_distance_tie_break_test)
     CHECK_EQUAL_RANGES(d.get_predecessors(), expected);
 }
 
+// Following the predecessors from anywhere reaches the source.
+//
+// This is what every caller of get_predecessors() does, and none of them can check it
+// first: recovering a path means walking the array until the root turns up. If two
+// entries point at each other the walk never ends, and in an extractor that means the
+// whole run never finishes, with no output and no error to look at.
+//
+// The shape that breaks it needs an edge of no length between two vertices the same
+// distance from the source, which sounds contrived and is not. Plaza rings routinely
+// carry two nodes at the same location, and a ring edge between them weighs zero.
+//
+// With one, both vertices settle at the same distance, and the tie-break that prefers
+// the lower-numbered predecessor then fires in both directions: each is a better
+// predecessor for the other than the source is, so each ends up pointing at the other.
+BOOST_AUTO_TEST_CASE(area_dijkstra_leaves_no_cycle_in_the_predecessors)
+{
+    // Vertex numbers deliberately out of step with insertion order, because the
+    // tie-break compares the vertices and the walk uses their indices.
+    Dijkstra<unsigned> d;
+    d.add_edge(10, 3, 1.0); // the source, and one arm
+    d.add_edge(10, 2, 1.0); // the other arm, exactly as long
+    d.add_edge(3, 2, 0.0);  // two nodes mapped at the same place
+
+    const auto source = d.index_of(10);
+    BOOST_CHECK_NO_THROW(d.run(source));
+
+    const auto &predecessors = d.get_predecessors();
+    for (std::size_t start = 0; start < d.num_vertices(); ++start)
+    {
+        std::size_t v = start;
+        std::size_t steps = 0;
+        while (v != source && v != predecessors.at(v))
+        {
+            v = predecessors.at(v);
+            BOOST_REQUIRE_MESSAGE(++steps <= d.num_vertices(),
+                                  "predecessors cycle, walking from vertex "
+                                      << d.get_vertex(start));
+        }
+    }
+
+    // Both arms are still a distance of one away, and the tie is still broken the same
+    // way every run: vertex 2 is reached through vertex 3, because going that way costs
+    // the same and 3 is the lower predecessor. Only the loop is gone, not the choice.
+    BOOST_CHECK_CLOSE(d.get_distances().at(d.index_of(3)), 1.0, 1e-9);
+    BOOST_CHECK_CLOSE(d.get_distances().at(d.index_of(2)), 1.0, 1e-9);
+    BOOST_CHECK_EQUAL(predecessors.at(d.index_of(3)), source);
+    BOOST_CHECK_EQUAL(predecessors.at(d.index_of(2)), d.index_of(3));
+}
+
 BOOST_AUTO_TEST_SUITE_END()