From 706210f78b2fe76095351787908301aedaf8d17e Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Fri, 15 May 2026 12:37:15 -0500 Subject: [PATCH 1/6] Make mip_node_t destructor exception-safe (SonarQube CRIT BUG cpp:S1048) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The iterative teardown uses `std::vector::push_back`, which can throw `std::bad_alloc`. Letting that propagate out of a destructor calls `std::terminate`. Wrap the body in a try/catch(...) so the destructor stays exception-free. Under OOM the catch absorbs the failure, and any descendants still attached to `children` are destroyed via the recursive unique_ptr chain as this frame unwinds — which risks stack overflow on a very deep tree, but only when memory is already exhausted, and is strictly better than `std::terminate`. Clears 2 CRITICAL bug findings on `main`: - cpp/src/branch_and_bound/mip_node.hpp:49 - cpp/src/branch_and_bound/mip_node.hpp:54 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- cpp/src/branch_and_bound/mip_node.hpp | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/cpp/src/branch_and_bound/mip_node.hpp b/cpp/src/branch_and_bound/mip_node.hpp index 694a7099c4..0240ad2446 100644 --- a/cpp/src/branch_and_bound/mip_node.hpp +++ b/cpp/src/branch_and_bound/mip_node.hpp @@ -44,18 +44,24 @@ class mip_node_t { { // Iterative teardown to avoid stack overflow on deep trees. // Detach all descendants breadth-first, then destroy them as leaves. - std::vector> nodes; - for (auto& c : children) { - if (c) { nodes.push_back(std::move(c)); } - } - // nodes.size() grows so that this loop only terminates when only leaves remain - for (size_t i = 0; i < nodes.size(); ++i) { - for (auto& c : nodes[i]->children) { + // vector::push_back can throw bad_alloc; the catch-all keeps the destructor + // exception-free. Under OOM, any not-yet-detached descendants are destroyed + // via the recursive unique_ptr chain in `children` as this frame unwinds. + try { + std::vector> nodes; + for (auto& c : children) { if (c) { nodes.push_back(std::move(c)); } } - } + // nodes.size() grows so that this loop only terminates when only leaves remain + for (size_t i = 0; i < nodes.size(); ++i) { + for (auto& c : nodes[i]->children) { + if (c) { nodes.push_back(std::move(c)); } + } + } - // scope-exit ensure destruction of all detached leaves + // scope-exit ensure destruction of all detached leaves + } catch (...) { + } } mip_node_t(mip_node_t&&) = default; From b08429f5ec708ef3c61d4a58d36cafe5014507d6 Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Fri, 15 May 2026 13:05:27 -0500 Subject: [PATCH 2/6] Also make timing_raii_t destructor exception-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codebase survey for other destructors that can throw bad_alloc found the same pattern in fj_cpu.cu's `timing_raii_t::~timing_raii_t` (push_back into a std::vector). SonarQube missed it because .cu files aren't covered by its C++ analyzer; addresses the broader concern from the PR review. Same try/catch(...) approach. Losing one timing sample under OOM is acceptable; the alternative is std::terminate. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu b/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu index 575228895b..8f183a4e46 100644 --- a/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu +++ b/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu @@ -230,10 +230,15 @@ class timing_raii_t { ~timing_raii_t() { - auto end_time = std::chrono::high_resolution_clock::now(); - auto duration = - std::chrono::duration_cast>(end_time - start_time_); - times_vec_.push_back(duration.count()); + // vector::push_back can throw bad_alloc; the catch-all keeps the destructor + // exception-free. Losing one timing sample under OOM is acceptable. + try { + auto end_time = std::chrono::high_resolution_clock::now(); + auto duration = + std::chrono::duration_cast>(end_time - start_time_); + times_vec_.push_back(duration.count()); + } catch (...) { + } } private: From fd66d5a82a378d0678778dc2ada98cc4d17e22b2 Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Fri, 15 May 2026 13:07:18 -0500 Subject: [PATCH 3/6] Also make grpc_client_t destructor exception-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stop_log_streaming() can throw: std::thread::join may throw std::system_error if the join fails, and std::lock_guard's mutex acquisition can throw on a mutex error. Wrap the call in try/catch(...) so the destructor stays exception-free. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- cpp/src/grpc/client/grpc_client.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cpp/src/grpc/client/grpc_client.cpp b/cpp/src/grpc/client/grpc_client.cpp index 59c6bfcb5d..6f22450c83 100644 --- a/cpp/src/grpc/client/grpc_client.cpp +++ b/cpp/src/grpc/client/grpc_client.cpp @@ -127,7 +127,15 @@ grpc_client_t::grpc_client_t(const std::string& server_address) : impl_(std::mak chunked_array_threshold_bytes_ = config_.max_message_bytes * 3 / 4; } -grpc_client_t::~grpc_client_t() { stop_log_streaming(); } +grpc_client_t::~grpc_client_t() +{ + // stop_log_streaming() can throw via std::thread::join (std::system_error) + // and std::lock_guard's mutex acquisition. Keep the destructor exception-free. + try { + stop_log_streaming(); + } catch (...) { + } +} bool grpc_client_t::connect() { From 9dcb39ea8a1d44c63fa73d985de1bb0b593ef551 Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Mon, 18 May 2026 12:58:40 -0500 Subject: [PATCH 4/6] Inline truly-noexcept shutdown into ~grpc_client_t MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous version wrapped stop_log_streaming() in try/catch in the destructor, but that does not actually prevent std::terminate: 1. If lock_guard or TryCancel throw before the swap, log_thread_ still owns a joinable std::thread; when the surrounding grpc_client_t destructor finishes and log_thread_ is destroyed, a joinable thread's destructor calls std::terminate — outside the catch block. 2. If t->join() throws, the local unique_ptr t is destroyed during the same unwind. Same problem — joinable thread destructor terminates before the catch can fire. Inline a noexcept shutdown in the destructor instead: cancel under a try/catch, swap log_thread_ into a local (so the member is now empty), join the local, and on join failure detach the thread before its destructor runs. The public stop_log_streaming() keeps its existing contract for the solve_lp / solve_mip callers. Thanks to CodeRabbit for the catch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- cpp/src/grpc/client/grpc_client.cpp | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/cpp/src/grpc/client/grpc_client.cpp b/cpp/src/grpc/client/grpc_client.cpp index 6f22450c83..54bd320237 100644 --- a/cpp/src/grpc/client/grpc_client.cpp +++ b/cpp/src/grpc/client/grpc_client.cpp @@ -129,11 +129,32 @@ grpc_client_t::grpc_client_t(const std::string& server_address) : impl_(std::mak grpc_client_t::~grpc_client_t() { - // stop_log_streaming() can throw via std::thread::join (std::system_error) - // and std::lock_guard's mutex acquisition. Keep the destructor exception-free. + // The destructor must not propagate exceptions AND must not leave a joinable + // std::thread alive — a joinable thread's destructor calls std::terminate. + // We inline a noexcept variant of stop_log_streaming() here so that on any + // failure we still detach the thread before its destructor runs. + stop_logs_.store(true); try { - stop_log_streaming(); + std::lock_guard lk(log_context_mutex_); + if (active_log_context_) { + static_cast(active_log_context_)->TryCancel(); + } } catch (...) { + // Best-effort cancel; fall through to join/detach the thread. + } + std::unique_ptr t; + std::swap(t, log_thread_); + if (t && t->joinable()) { + try { + t->join(); + } catch (...) { + // join failed (e.g., std::system_error). Detach so the local + // unique_ptr's destructor doesn't terminate on the joinable thread. + try { + t->detach(); + } catch (...) { + } + } } } From c27e8f6aaf14c56538fabd021fffdd466215cdbc Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Mon, 18 May 2026 14:34:09 -0500 Subject: [PATCH 5/6] Log via CUOPT_LOG_ERROR instead of silently swallowing in destructor catches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review on PR #1230: silently catching in destructors hides real failures. Switch each catch block to log via CUOPT_LOG_ERROR with the exception's what(), matching the pattern used in solve.cu. The catch (...) is kept as a noexcept safety net. (Cannot literally throw RuntimeError as the review suggested — that would re-introduce the std::terminate path this PR exists to close. Logging is the closest we can get while satisfying the destructor noexcept contract.) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- cpp/src/branch_and_bound/mip_node.hpp | 6 ++++++ cpp/src/grpc/client/grpc_client.cpp | 15 ++++++++++++++- cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu | 2 ++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/cpp/src/branch_and_bound/mip_node.hpp b/cpp/src/branch_and_bound/mip_node.hpp index 0240ad2446..3fe3fcd7fd 100644 --- a/cpp/src/branch_and_bound/mip_node.hpp +++ b/cpp/src/branch_and_bound/mip_node.hpp @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -60,6 +61,11 @@ class mip_node_t { } // scope-exit ensure destruction of all detached leaves + } catch (const std::exception& e) { + CUOPT_LOG_ERROR( + "mip_node_t destructor: iterative teardown failed (%s); falling back to " + "recursive unique_ptr destruction.", + e.what()); } catch (...) { } } diff --git a/cpp/src/grpc/client/grpc_client.cpp b/cpp/src/grpc/client/grpc_client.cpp index 54bd320237..6eb560247e 100644 --- a/cpp/src/grpc/client/grpc_client.cpp +++ b/cpp/src/grpc/client/grpc_client.cpp @@ -139,6 +139,11 @@ grpc_client_t::~grpc_client_t() if (active_log_context_) { static_cast(active_log_context_)->TryCancel(); } + } catch (const std::exception& e) { + CUOPT_LOG_ERROR( + "grpc_client_t destructor: TryCancel/lock failed (%s); proceeding to " + "join/detach.", + e.what()); } catch (...) { // Best-effort cancel; fall through to join/detach the thread. } @@ -147,13 +152,21 @@ grpc_client_t::~grpc_client_t() if (t && t->joinable()) { try { t->join(); - } catch (...) { + } catch (const std::exception& e) { + CUOPT_LOG_ERROR("grpc_client_t destructor: log-thread join failed (%s); detaching.", + e.what()); // join failed (e.g., std::system_error). Detach so the local // unique_ptr's destructor doesn't terminate on the joinable thread. try { t->detach(); + } catch (const std::exception& e2) { + CUOPT_LOG_ERROR( + "grpc_client_t destructor: detach also failed (%s); thread may " + "terminate the process on unique_ptr destruction.", + e2.what()); } catch (...) { } + } catch (...) { } } } diff --git a/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu b/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu index 8f183a4e46..68dad624ae 100644 --- a/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu +++ b/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu @@ -237,6 +237,8 @@ class timing_raii_t { auto duration = std::chrono::duration_cast>(end_time - start_time_); times_vec_.push_back(duration.count()); + } catch (const std::exception& e) { + CUOPT_LOG_ERROR("timing_raii_t destructor: failed to record sample (%s).", e.what()); } catch (...) { } } From 27dc764f375a97201d6ba8784a83583955b84d5b Mon Sep 17 00:00:00 2001 From: Ramakrishna Prabhu Date: Wed, 20 May 2026 17:11:49 -0500 Subject: [PATCH 6/6] Use fprintf(stderr) in noexcept destructor catches Per hlinsen's review on PR #1230: CUOPT_LOG_ERROR can allocate, and a secondary bad_alloc raised from inside one of these catch handlers would escape the destructor and re-introduce std::terminate -- exactly the path this PR exists to close. fprintf to stderr is allocation-free and cannot throw, so it's safe to call from any destructor catch handler regardless of memory pressure. Also fill in previously-empty catch(...) blocks with stderr messages so non-std::exception failures are observable instead of silently absorbed, and add a missing detach() attempt on the catch(...) path in ~grpc_client_t so a non-std::exception thrown from join() doesn't leave a joinable thread that would terminate the process when the local unique_ptr destructs. Co-Authored-By: Claude Opus 4.7 (1M context) --- cpp/src/branch_and_bound/mip_node.hpp | 16 ++++--- cpp/src/grpc/client/grpc_client.cpp | 43 ++++++++++++++----- .../mip_heuristics/feasibility_jump/fj_cpu.cu | 8 +++- 3 files changed, 50 insertions(+), 17 deletions(-) diff --git a/cpp/src/branch_and_bound/mip_node.hpp b/cpp/src/branch_and_bound/mip_node.hpp index 3fe3fcd7fd..2b2336d1ed 100644 --- a/cpp/src/branch_and_bound/mip_node.hpp +++ b/cpp/src/branch_and_bound/mip_node.hpp @@ -13,10 +13,10 @@ #include #include -#include #include #include +#include #include #include #include @@ -62,11 +62,17 @@ class mip_node_t { // scope-exit ensure destruction of all detached leaves } catch (const std::exception& e) { - CUOPT_LOG_ERROR( - "mip_node_t destructor: iterative teardown failed (%s); falling back to " - "recursive unique_ptr destruction.", - e.what()); + // fprintf to stderr is allocation-free and cannot throw; using the + // project logger here would risk a secondary bad_alloc that would + // escape the destructor and re-introduce std::terminate. + std::fprintf(stderr, + "mip_node_t destructor: iterative teardown failed (%s); falling back to " + "recursive unique_ptr destruction.\n", + e.what()); } catch (...) { + std::fprintf(stderr, + "mip_node_t destructor: iterative teardown failed (unknown exception); " + "falling back to recursive unique_ptr destruction.\n"); } } diff --git a/cpp/src/grpc/client/grpc_client.cpp b/cpp/src/grpc/client/grpc_client.cpp index 6eb560247e..060b76f79c 100644 --- a/cpp/src/grpc/client/grpc_client.cpp +++ b/cpp/src/grpc/client/grpc_client.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -133,6 +134,11 @@ grpc_client_t::~grpc_client_t() // std::thread alive — a joinable thread's destructor calls std::terminate. // We inline a noexcept variant of stop_log_streaming() here so that on any // failure we still detach the thread before its destructor runs. + // + // fprintf to stderr is used instead of CUOPT_LOG_ERROR because the project + // logger can allocate, and a secondary bad_alloc raised from inside one of + // these catch handlers would escape the destructor and re-introduce + // std::terminate — defeating the purpose of catching at all. stop_logs_.store(true); try { std::lock_guard lk(log_context_mutex_); @@ -140,12 +146,14 @@ grpc_client_t::~grpc_client_t() static_cast(active_log_context_)->TryCancel(); } } catch (const std::exception& e) { - CUOPT_LOG_ERROR( - "grpc_client_t destructor: TryCancel/lock failed (%s); proceeding to " - "join/detach.", - e.what()); + std::fprintf(stderr, + "grpc_client_t destructor: TryCancel/lock failed (%s); proceeding to " + "join/detach.\n", + e.what()); } catch (...) { - // Best-effort cancel; fall through to join/detach the thread. + std::fprintf(stderr, + "grpc_client_t destructor: TryCancel/lock failed (unknown exception); " + "proceeding to join/detach.\n"); } std::unique_ptr t; std::swap(t, log_thread_); @@ -153,20 +161,33 @@ grpc_client_t::~grpc_client_t() try { t->join(); } catch (const std::exception& e) { - CUOPT_LOG_ERROR("grpc_client_t destructor: log-thread join failed (%s); detaching.", - e.what()); + std::fprintf( + stderr, "grpc_client_t destructor: log-thread join failed (%s); detaching.\n", e.what()); // join failed (e.g., std::system_error). Detach so the local // unique_ptr's destructor doesn't terminate on the joinable thread. try { t->detach(); } catch (const std::exception& e2) { - CUOPT_LOG_ERROR( - "grpc_client_t destructor: detach also failed (%s); thread may " - "terminate the process on unique_ptr destruction.", - e2.what()); + std::fprintf(stderr, + "grpc_client_t destructor: detach also failed (%s); thread may " + "terminate the process on unique_ptr destruction.\n", + e2.what()); } catch (...) { + std::fprintf(stderr, + "grpc_client_t destructor: detach also failed (unknown exception); " + "thread may terminate the process on unique_ptr destruction.\n"); } } catch (...) { + std::fprintf(stderr, + "grpc_client_t destructor: log-thread join failed (unknown exception); " + "detaching.\n"); + try { + t->detach(); + } catch (...) { + std::fprintf(stderr, + "grpc_client_t destructor: detach also failed; thread may " + "terminate the process on unique_ptr destruction.\n"); + } } } } diff --git a/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu b/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu index 68dad624ae..a537426457 100644 --- a/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu +++ b/cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu @@ -25,6 +25,7 @@ #include #include #include +#include #include #include #include @@ -232,14 +233,19 @@ class timing_raii_t { { // vector::push_back can throw bad_alloc; the catch-all keeps the destructor // exception-free. Losing one timing sample under OOM is acceptable. + // fprintf to stderr is allocation-free and cannot throw; using the project + // logger here would risk a secondary bad_alloc that would escape the + // destructor and re-introduce std::terminate. try { auto end_time = std::chrono::high_resolution_clock::now(); auto duration = std::chrono::duration_cast>(end_time - start_time_); times_vec_.push_back(duration.count()); } catch (const std::exception& e) { - CUOPT_LOG_ERROR("timing_raii_t destructor: failed to record sample (%s).", e.what()); + std::fprintf(stderr, "timing_raii_t destructor: failed to record sample (%s).\n", e.what()); } catch (...) { + std::fprintf(stderr, + "timing_raii_t destructor: failed to record sample (unknown exception).\n"); } }