Fix race condition in RINS and lost nodes in branch and bound

cpp/src/branch_and_bound/branch_and_bound.cpp

@@ -1436,8 +1436,9 @@ void branch_and_bound_t<i_t, f_t>::plunge_with(branch_and_bound_worker_t<i_t, f_
 {
   std::deque<mip_node_t<i_t, f_t>*> stack;
   stack.push_front(worker->start_node);
-  worker->recompute_basis  = true;
-  worker->recompute_bounds = true;
+  bool requeue_pending_nodes = false;
+  worker->recompute_basis    = true;
+  worker->recompute_bounds   = true;
 
   f_t lower_bound = get_lower_bound();
   f_t upper_bound = upper_bound_;
@@ -1483,8 +1484,11 @@ void branch_and_bound_t<i_t, f_t>::plunge_with(branch_and_bound_worker_t<i_t, f_
       break;
     } else if (lp_status == dual::status_t::CONCURRENT_LIMIT) {
       stack.push_front(node_ptr);
+      requeue_pending_nodes = true;
       break;
     } else if (lp_status == dual::status_t::ITERATION_LIMIT) {
+      stack.push_front(node_ptr);
+      requeue_pending_nodes = true;
       break;
     }
 
@@ -1541,11 +1545,11 @@ void branch_and_bound_t<i_t, f_t>::plunge_with(branch_and_bound_worker_t<i_t, f_
   rel_gap     = user_relative_gap(original_lp_, upper_bound, lower_bound);
   abs_gap     = compute_user_abs_gap(original_lp_, upper_bound, lower_bound);
 
-  if (stack.size() > 0 &&
-      (rel_gap <= settings_.relative_mip_gap_tol || abs_gap <= settings_.absolute_mip_gap_tol)) {
-    // If the solver converged according to the gap rules, but we still have nodes to explore
-    // in the stack, then we should add all the pending nodes back to the heap so the lower
-    // bound of the solver is set to the correct value.
+  if (stack.size() > 0 && (requeue_pending_nodes || rel_gap <= settings_.relative_mip_gap_tol ||
+                           abs_gap <= settings_.absolute_mip_gap_tol)) {
+    // If the solver exits early without consuming the local stack, or converged according to
+    // the gap rules while nodes are still pending, put those nodes back into the global queue
+    // before returning.
     while (!stack.empty()) {
       auto node = stack.front();
       stack.pop_front();

cpp/src/mip_heuristics/diversity/lns/rins.cu

@@ -26,6 +26,12 @@
 #include <dual_simplex/tic_toc.hpp>
 
 namespace cuopt::linear_programming::detail {
+template <typename i_t, typename f_t>
+rins_t<i_t, f_t>::~rins_t()
+{
+  stop_rins();
+}
+
 template <typename i_t, typename f_t>
 rins_t<i_t, f_t>::rins_t(mip_solver_context_t<i_t, f_t>& context_,
                          diversity_manager_t<i_t, f_t>& dm_,
@@ -58,31 +64,35 @@ void rins_t<i_t, f_t>::new_best_incumbent_callback(const std::vector<f_t>& solut
 template <typename i_t, typename f_t>
 void rins_t<i_t, f_t>::node_callback(const std::vector<f_t>& solution, f_t objective)
 {
-  if (!enabled) return;
+  if (!enabled.load()) return;
 
   node_count++;
 
   if (node_count - node_count_at_last_improvement < settings.nodes_after_later_improvement) return;
 
-  if (node_count - node_count_at_last_rins > settings.node_freq) {
-    // opportunistic early test w/ atomic to avoid having to take the lock
-    if (!rins_thread->cpu_thread_done) return;
-    std::lock_guard<std::mutex> lock(rins_mutex);
-    bool population_ready = false;
-    if (rins_thread->cpu_thread_done) {
-      std::lock_guard<std::recursive_mutex> pop_lock(dm.population.write_mutex);
-      population_ready = dm.population.current_size() > 0 && dm.population.is_feasible();
-    }
-    if (population_ready) {
-      lp_optimal_solution = solution;
-      rins_thread->start_cpu_solver();
-    }
+  if (node_count - node_count_at_last_rins <= settings.node_freq) { return; }
+
+  std::lock_guard<std::mutex> lock(rins_mutex);
+  if (!enabled.load() || !rins_thread) { return; }
+  if (!rins_thread->cpu_thread_done.load()) { return; }
+
+  {
+    std::lock_guard<std::recursive_mutex> pop_lock(dm.population.write_mutex);
+    if (dm.population.current_size() == 0 || !dm.population.is_feasible()) { return; }
+
+    auto& best_feasible_ref = dm.population.best_feasible();
+    if (!best_feasible_ref.get_feasible()) { return; }
+
+    incumbent_solution_snapshot = best_feasible_ref.get_host_assignment();
+    lp_optimal_solution         = solution;
   }
+  rins_thread->start_cpu_solver();
 }
 
 template <typename i_t, typename f_t>
 void rins_t<i_t, f_t>::enable()
 {
+  std::lock_guard<std::mutex> lock(rins_mutex);
   rins_thread           = std::make_unique<rins_thread_t<i_t, f_t>>();
   rins_thread->rins_ptr = this;
   seed                  = cuopt::seed_generator::get_seed();
@@ -94,17 +104,20 @@ void rins_t<i_t, f_t>::enable()
 template <typename i_t, typename f_t>
 void rins_t<i_t, f_t>::stop_rins()
 {
-  enabled = false;
-  if (rins_thread) rins_thread->request_termination();
-  rins_thread.reset();
+  std::unique_ptr<rins_thread_t<i_t, f_t>> local_thread;
+  {
+    std::lock_guard<std::mutex> lock(rins_mutex);
+    enabled      = false;
+    local_thread = std::move(rins_thread);
+  }
+  if (local_thread) { local_thread->request_termination(); }
 }
 
 template <typename i_t, typename f_t>
 void rins_t<i_t, f_t>::run_rins()
 {
   if (total_calls == 0) RAFT_CUDA_TRY(cudaSetDevice(context.handle_ptr->get_device()));
 
-  cuopt_assert(lp_optimal_solution.size() == problem_copy->n_variables, "Assignment size mismatch");
   cuopt_assert(problem_copy->handle_ptr == &rins_handle, "Handle mismatch");
   // Do not make assertions based on problem_ptr. The original problem may have been modified within
   // the FP loop relaxing integers cuopt_assert(problem_copy->n_variables ==
@@ -117,20 +130,11 @@ void rins_t<i_t, f_t>::run_rins()
 
   solution_t<i_t, f_t> best_sol(*problem_copy);
   rins_handle.sync_stream();
-  // copy the best from the population into a solution_t in the RINS stream
-  {
-    std::lock_guard<std::recursive_mutex> lock(dm.population.write_mutex);
-    if (!dm.population.is_feasible()) return;
-    cuopt_assert(dm.population.current_size() > 0, "No solutions in population");
-    auto& best_feasible_ref = dm.population.best_feasible();
-    cuopt_assert(best_feasible_ref.assignment.size() == best_sol.assignment.size(),
-                 "Assignment size mismatch");
-    cuopt_assert(best_feasible_ref.get_feasible(), "Best feasible is not feasible");
-    expand_device_copy(best_sol.assignment, best_feasible_ref.assignment, rins_handle.get_stream());
-    best_sol.handle_ptr  = &rins_handle;
-    best_sol.problem_ptr = problem_copy.get();
-    best_sol.compute_feasibility();
-  }
+  // Use the launch-time snapshot to keep the incumbent and problem model consistent.
+  best_sol.copy_new_assignment(incumbent_solution_snapshot);
+  best_sol.handle_ptr  = &rins_handle;
+  best_sol.problem_ptr = problem_copy.get();
+  best_sol.compute_feasibility();
   cuopt_assert(best_sol.handle_ptr == &rins_handle, "Handle mismatch");
 
   cuopt_assert(best_sol.get_feasible(), "Best solution is not feasible");

cpp/src/mip_heuristics/diversity/lns/rins.cuh

@@ -67,6 +67,7 @@ struct rins_thread_t : public cpu_worker_thread_base_t<rins_thread_t<i_t, f_t>>
 template <typename i_t, typename f_t>
 class rins_t {
  public:
+  ~rins_t();
   rins_t(mip_solver_context_t<i_t, f_t>& context,
          diversity_manager_t<i_t, f_t>& dm,
          rins_settings_t settings = rins_settings_t());
@@ -89,6 +90,7 @@ class rins_t {
   raft::handle_t rins_handle;
 
   std::vector<f_t> lp_optimal_solution;
+  std::vector<f_t> incumbent_solution_snapshot;
 
   f_t fixrate{0.5};
   i_t total_calls{0};