Skip to content

fix lifetime bug with presolve result#907

Closed
tmckayus wants to merge 2 commits intoNVIDIA:mainfrom
tmckayus:fix/presolve-result-lifetime
Closed

fix lifetime bug with presolve result#907
tmckayus wants to merge 2 commits intoNVIDIA:mainfrom
tmckayus:fix/presolve-result-lifetime

Conversation

@tmckayus
Copy link
Copy Markdown
Contributor

@tmckayus tmckayus commented Feb 26, 2026

During local development and testing I had reproducible crashes during presolve tests. This appears to be because the presolve result was declared in an inner scope, but the reduced problem is referenced by problem.original_problem_ptr in an outer block at line 1279.

I have not seen this in CI, but timing and memory layout on my local machine may have exposed a latent issue. This change resolved the crash for me.

Summary by CodeRabbit

  • Refactor
    • Reorganized variable scope management in the solver to improve clarity; clarified lifetime and error-flow with comments while preserving existing behavior.
    • Enabled default move semantics for optimization problem objects to allow efficient resource transfers without copying.
    • Preserved external interfaces and runtime behavior.

@tmckayus tmckayus requested a review from a team as a code owner February 26, 2026 17:24
@tmckayus tmckayus requested review from Bubullzz and rg20 February 26, 2026 17:24
@tmckayus tmckayus added bug Something isn't working non-breaking Introduces a non-breaking change labels Feb 26, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Feb 26, 2026

📝 Walkthrough

Walkthrough

Moved a presolver result variable in the PDLP solver from inner to outer scope to persist its value across control flow, and added defaulted move constructor and move assignment to the optimization_problem_t class to enable move semantics; no other behavioral changes introduced.

Changes

Cohort / File(s) Summary
PDLP solver scope change
cpp/src/pdlp/solve.cu
Presolver result variable relocated from inner/local scope to an outer optional to retain value across control flow; assignment updated and clarifying comment about lifetime/reduced-problem reference added.
Optimization problem move semantics
cpp/include/cuopt/linear_programming/optimization_problem.hpp
Added defaulted move constructor and move assignment operator to optimization_problem_t to enable move semantics (optimization_problem_t(optimization_problem_t&&) = default, operator=(optimization_problem_t&&) = default).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 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 (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the main change: fixing a lifetime bug with the presolve result, which is precisely what the changeset addresses.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

@anandhkb anandhkb added this to the 26.04 milestone Feb 26, 2026
@tmckayus tmckayus force-pushed the fix/presolve-result-lifetime branch from 65afe84 to d21b147 Compare February 26, 2026 21:49
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
cpp/src/pdlp/solve.cu (1)

1206-1206: Add an explicit <optional> include to cpp/src/pdlp/solve.cu.

std::optional is used directly at line 1206; relying on transitive includes from third_party_presolve.hpp is brittle and can break with unrelated header changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cpp/src/pdlp/solve.cu` at line 1206, Add an explicit include for the
<optional> header in the file that defines the use of std::optional (so that
std::optional is available for the declaration of
std::optional<detail::third_party_presolve_result_t<i_t, f_t>> result;). Locate
the top-of-file includes in solve.cu and insert `#include` <optional> (before or
alongside other standard headers) so the code no longer relies on transitive
includes from third_party_presolve.hpp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@cpp/src/pdlp/solve.cu`:
- Line 1206: Add an explicit include for the <optional> header in the file that
defines the use of std::optional (so that std::optional is available for the
declaration of std::optional<detail::third_party_presolve_result_t<i_t, f_t>>
result;). Locate the top-of-file includes in solve.cu and insert `#include`
<optional> (before or alongside other standard headers) so the code no longer
relies on transitive includes from third_party_presolve.hpp.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 65afe84 and d21b147.

📒 Files selected for processing (2)
  • cpp/include/cuopt/linear_programming/optimization_problem.hpp
  • cpp/src/pdlp/solve.cu

@tmckayus
Copy link
Copy Markdown
Contributor Author

actually it looks like this exact fix is need on #908

perhaps the memory model changes set up the right conditions

@tmckayus
Copy link
Copy Markdown
Contributor Author

this change was included in #908

@tmckayus tmckayus closed this Feb 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants