Make mip_node_t destructor exception-safe (SonarQube CRIT BUG cpp:S1048)

[rgsl888prabhu]

Summary

The iterative-teardown destructor in mip_node_t uses std::vector::push_back to walk and detach the subtree. push_back can throw std::bad_alloc, and a destructor that lets an exception propagate calls std::terminate.

Wraps the body in 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 under OOM, but is strictly better than std::terminate.

[coderabbitai]

📝 Walkthrough

Walkthrough

Three destructors were made exception-tolerant: mip_node_t’s breadth-first descendant detachment, timing_raii_t’s timing-sample push, and grpc_client_t’s stop_log_streaming call are now wrapped in catch-all handlers so exceptions (e.g., OOM or join/lock failures) won’t propagate from destructors.

Changes

Destructor exception-safety

Layer / File(s)	Summary
mip_node_t destructor wrapper cpp/src/branch_and_bound/mip_node.hpp	The breadth-first detached-descendants collection/expansion loop in mip_node_t's destructor is wrapped in try { ... } catch(...) to suppress exceptions from std::vector::push_back; remaining descendants are destroyed via the unique_ptr chain during stack unwinding.
timing_raii_t destructor wrapper cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu	The timing_raii_t destructor now catches all exceptions around times_vec_.push_back(...), preventing exceptions (e.g., OOM) from escaping the destructor when recording the measured duration.
grpc_client_t destructor wrapper cpp/src/grpc/client/grpc_client.cpp	grpc_client_t::~grpc_client_t() now wraps stop_log_streaming() in try { ... } catch(...) to ensure destructor does not propagate exceptions from thread join or mutex operations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

bug

Suggested reviewers

• kaatish

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: making the mip_node_t destructor exception-safe to address a SonarQube critical bug.
Description check	✅ Passed	The description is directly related to the changeset, explaining the motivation (std::bad_alloc in push_back), the solution (try/catch wrapper), and expected behavior under OOM conditions.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/sonar-bug-mip-node-destructor-noexcept

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[mlubin]

Do we intent to fail nicely on every potential line that could throw bad_alloc? I doubt this is the only spot that would need to change if that's true.

[rgsl888prabhu]

Good question — I shouldn't have framed this as "fail nicely on bad_alloc." The narrower motivation is the destructor contract specifically: since C++11, destructors are implicitly noexcept, so an escaping exception calls std::terminate immediately, with no chance for a handler. In non-destructor code an unhandled bad_alloc can still propagate to a caller that may choose what to do. Rule cpp:S1048 targets only this destructor case.

I also surveyed cpp/{include,src} for other destructors that allocate or otherwise can throw. Results, and what's now in this PR:

File	Destructor	Status
cpp/src/branch_and_bound/mip_node.hpp	mip_node_t	Fixed in this PR (push_back → bad_alloc)
cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu	timing_raii_t	Fixed in this PR (same push_back pattern)
cpp/src/grpc/client/grpc_client.cpp	grpc_client_t (via stop_log_streaming)	Fixed in this PR (thread::join → system_error, lock_guard ctor)
cpp/src/utilities/high_res_timer.hpp	HighResTimer	Body is empty — safe

SonarQube only flagged mip_node_t because .cu files aren't covered by its C++ analyzer, and it doesn't reason about thread::join propagating from the called stop_log_streaming(). That's the gap your question implicitly pointed at.

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 6e1fa19c-ebee-4801-9e5f-7248286561d0

📥 Commits

Reviewing files that changed from the base of the PR and between b08429f and fd66d5a.

📒 Files selected for processing (1)

• cpp/src/grpc/client/grpc_client.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="cpp/src/grpc/client/grpc_client.cpp"

echo "=== Destructor wrapper ==="
sed -n '130,140p' "$FILE"

echo
echo "=== stop_log_streaming implementation ==="
sed -n '238,257p' "$FILE"

Repository: NVIDIA/cuopt

Length of output: 1157

---

Destructor catch-all cannot prevent std::terminate() on exception paths.

The try-catch wrapper in the destructor (lines 130–138) does not fully protect against process termination. Two exception scenarios remain unsafe:

1. Exception before std::swap(t, log_thread_) (e.g., at std::lock_guard<std::mutex> or TryCancel()): The member log_thread_ still owns the thread. When the destructor exits and log_thread_ is destroyed, if it holds a joinable thread, its destructor calls std::terminate() — occurring outside the catch block.

2. Exception at t->join(): During stack unwinding, the local std::unique_ptr<std::thread> t is destroyed before the exception exits stop_log_streaming(). If the thread is joinable, std::terminate() is invoked during unwinding, before the catch block can suppress it.

The suggested fix—a stop_log_streaming_noexcept() method that guarantees no exceptions and no joinable thread ownership on any exit path—is the correct approach to satisfy the exception-safe destructor requirement.

[mlubin]

I'll defer approval to others who have more familiarity with this code.