Skip to content

Fix cuopt_mvn retry loop being silently skipped under set -e - #1823

Merged
rapids-bot[bot] merged 2 commits into
mainfrom
fix-java-mvn-retry-set-e
Aug 28, 2026
Merged

Fix cuopt_mvn retry loop being silently skipped under set -e#1823
rapids-bot[bot] merged 2 commits into
mainfrom
fix-java-mvn-retry-set-e

Conversation

@ramakrishnap-nv

Copy link
Copy Markdown
Collaborator

Summary

  • cuopt_mvn's retry/backoff loop (java/cuopt/scripts/maven.sh, added in Java bindings for LP, MIP and QP #1524 to address java-build fails intermittently on Maven Central 429 rate limiting #1820) never actually ran: test.sh invokes it under set -euo pipefail, and the loop ran mvn ... | tee "${log}" as a bare statement rather than as the condition of an if/while. With pipefail on, a failing mvn makes the pipeline's exit status non-zero, and set -e then kills the function immediately — before the code that reads PIPESTATUS[0] and decides whether to retry ever runs.
  • Confirmed against a live failure (job 98796785653, PR Concurrent halt fix on barrier #1810, 2026-08-28): a single mvn attempt hits a Maven Central 429, and the job fails immediately with none of cuopt_mvn's retry log lines present, even though that branch already had the retry code.
  • Fix: guard the pipeline as the condition of an if so its failure is caught by the loop instead of triggering the caller's set -e. Verified locally that the loop now retries with backoff on a simulated 429 and still returns cleanly on success.

Test plan

  • java-build CI passes
  • Local repro: stubbed mvn returning a 429-style error under set -euo pipefail now produces mvn attempt 1/3 ..., attempt 2/3 ..., backoff sleeps, and a final failure only after exhausting retries (previously died on attempt 1 with no retry log at all)
  • Local repro: stubbed mvn returning success still returns 0 immediately

🤖 Generated with Claude Code

cuopt_mvn's retry loop ran `mvn ... | tee "${log}"` as a bare statement.
Callers (java/cuopt/scripts/test.sh) run under `set -euo pipefail`, so a
failing pipeline there aborted the function immediately, before the
retry logic that inspects PIPESTATUS ever ran. A Maven Central 429 hit
would fail on the very first attempt with none of the retry/backoff
behavior actually taking effect, as seen in job 98796785653 (PR #1810).

Guard the pipeline as the condition of an if so its failure is caught
by the retry loop instead of triggering the caller's set -e.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ramakrishnap-nv
ramakrishnap-nv requested a review from a team as a code owner August 28, 2026 13:33
@ramakrishnap-nv
ramakrishnap-nv requested a review from Iroy30 August 28, 2026 13:33
@ramakrishnap-nv ramakrishnap-nv self-assigned this Aug 28, 2026
@ramakrishnap-nv ramakrishnap-nv added bug Something isn't working non-breaking Introduces a non-breaking change labels Aug 28, 2026
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5e2cdff5-e984-4cb7-8d04-5d6896592c8f

📥 Commits

Reviewing files that changed from the base of the PR and between e75be62 and 6ab04ab.

📒 Files selected for processing (1)
  • java/cuopt/scripts/maven.sh

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The Maven wrapper now guards the Maven and tee pipeline with a conditional. It captures Maven's exit status through PIPESTATUS and continues to explicit retry and error handling.

Changes

Maven retry handling

Layer / File(s) Summary
Pipeline failure control
java/cuopt/scripts/maven.sh
cuopt_mvn prevents set -e from exiting before status capture. Successful runs set status to zero, while failed runs retain Maven's pipeline status for retry and error handling.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 6ab04

This localized change restores the intended Maven retry behavior under strict shell error handling. No actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: iroy30, afender, tmckayus, nvidiacbrissette

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
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.
Description check ✅ Passed The description clearly explains the cuopt_mvn retry failure under set -euo pipefail, the pipeline guard fix, and the validation performed.
Title check ✅ Passed The title clearly identifies the main change: preventing the cuopt_mvn retry loop from being skipped under set -e.
✨ 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-java-mvn-retry-set-e

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

@github-actions

Copy link
Copy Markdown

CI Test Summary

⏭️ All 5 test job(s) skipped.

@ramakrishnap-nv

Copy link
Copy Markdown
Collaborator Author

/merge

@rapids-bot
rapids-bot Bot merged commit 05a208e into main Aug 28, 2026
34 checks passed
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