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]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 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 shutdown/join sequence now swallow exceptions so destructors do not throw.

Changes

Destructor exception-safety

Layer / File(s)	Summary
mip_node_t destructor wrapper cpp/src/branch_and_bound/mip_node.hpp	Added logger include and wrapped the breadth-first detached-descendants collection/expansion loop in try { ... } catch(...); std::exception is logged, other exceptions are swallowed; remaining descendants are destroyed via the unique_ptr chain.
timing_raii_t destructor wrapper cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu	The timing_raii_t destructor now computes duration and calls times_vec_.push_back(...) inside try/catch; std::exception is logged and other exceptions are suppressed so the destructor is non-throwing.
grpc_client_t destructor shutdown cpp/src/grpc/client/grpc_client.cpp	grpc_client_t::~grpc_client_t() now sets stop_logs_, best-effort cancels active_log_context_ under log_context_mutex_ via TryCancel() (exceptions logged/suppressed), swaps out log_thread_, attempts join() in a try/catch, and falls back to detach() (also try/catch + logging) to avoid termination on join/detach failures.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

bug

Suggested reviewers

• hlinsen
• nguidotti

🚥 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 accurately summarizes the main change: making mip_node_t's destructor exception-safe to address a SonarQube critical bug (cpp:S1048).
Description check	✅ Passed	The description clearly explains the problem (push_back throwing std::bad_alloc in destructor causing std::terminate), the solution (wrapping in try/catch), and the tradeoff (possible stack overflow on deep trees under OOM, but better than termination).
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 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

[mlubin]

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

[rg20]

Looks like you are catching the exceptions but not reporting them. You can use cuopt_expects and throw RunTimeError.

[rgsl888prabhu]

@rg20 Good point — silently swallowing hides real failures. Switched each catch in this PR to log via CUOPT_LOG_ERROR("…: %s", e.what()), matching the pattern used in cpp/src/mip_heuristics/solve.cu (commit c27e8f6a).

One thing I could not literally do: throw RuntimeError from these catch blocks. Re-throwing out of a destructor would re-introduce the exact std::terminate path this PR exists to close (destructors are implicitly noexcept since C++11, so any escaping exception aborts the process). Logging is the closest we can get while keeping the destructor exception-free. If you’d rather we also surface these via cuopt_expects somewhere upstream of the destructor — e.g., add a noexcept teardown helper that callers like solve_lp/solve_mip use, and have that throw — happy to do that as a follow-up.

[nguidotti]

IMO we should move this logic to search_tree_t since it is where the entire tree is stored, not in the mip_node_t. For saving memory I would first switch to a depth-first search instead of bread-first and destroy the parent after moving the children to the stack.

[rgsl888prabhu]

IMO we should move this logic to search_tree_t since it is where the entire tree is stored, not in the mip_node_t. For saving memory I would first switch to a depth-first search instead of bread-first and destroy the parent after moving the children to the stack.

@nguidotti We may have to address this in a follow-up PR

[hlinsen]

CUOPT_LOG_ERROR can throw with allocations. I think it's best to use std::cerr inside the catch block

[rgsl888prabhu]

Using fprintf instead of std:cerr

            Property             │                                       fprintf(stderr, ...)                                       │                                                       std::cerr <<                                                       │
  ├──────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ Allocation                       │ Uses libc's pre-allocated FILE buffer for the format. Proven not to allocate for simple formats. │ Sentry construction and locale facet calls can allocate. Standard doesn't forbid it.                                     │
  ├──────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ Can throw?                       │ No — it's C, returns error code on failure.                                                      │ Yes, if cerr.exceptions(...) was ever flipped on transitively (off by default, but you'd be trusting the whole process). │
  ├──────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ Compatibility with existing code │ Same "%s" format strings already in the PR — mechanical swap.                                    │ Requires rewriting to << chains.