Fix cuopt_mvn retry loop being silently skipped under set -e by ramakrishnap-nv · Pull Request #1823 · NVIDIA/cuopt

ramakrishnap-nv · 2026-08-28T13:33:18Z

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>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

coderabbitai · 2026-08-28T13:35:03Z

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 · 2026-08-28T13:35:18Z

CI Test Summary

⏭️ All 5 test job(s) skipped.

ramakrishnap-nv · 2026-08-28T14:21:48Z

/merge

java-static-test has repeatedly hit 429 Too Many Requests resolving plugins like maven-source-plugin from a cold repository -- the cuopt_mvn wrapper's retry loop (fixed in #1823) retries the whole mvn invocation with backoff, but that's compensating for Maven's own resolver never being tuned, and 4 attempts don't reliably outlast a sustained rate limit. cuDF and cuVS already carry this exact fix for their own Java/Maven Central builds: a project-level .mvn/maven.config (auto-applied to every mvn invocation, no wrapper needed) that caps concurrent downloads to reduce burst request rate and adds a real backoff inside Maven's own transport-layer retry handler, rather than only retrying around the outside of a failed process: -Daether.connector.basic.downstreamThreads=1 -Daether.transport.http.retryHandler.count=5 -Daether.transport.http.retryHandler.interval=10000 -Dmaven.wagon.http.retryHandler.count=5 cuopt_mvn's own -D flags target the connector-layer retry handler, which recent Maven resolver versions may no longer consult now that retry logic lives at the transport layer -- this adds the layer that actually gets read, verified via `mvn help:evaluate -Dexpression=aether.transport.http.retryHandler.interval` resolving to 10000. Verified the packaged-jar-tests suite still passes with this config present.

…ing (#1835) ## Summary - `java-static-test` (from #1818) and, less often, `java-build` have hit `429 Too Many Requests` from Maven Central while resolving plugins like `maven-source-plugin` on a cold repository. `cuopt_mvn`'s retry loop (fixed in #1823 to actually run under `set -e`) retries the whole `mvn` invocation with backoff, but that's compensating for Maven's own resolver never being tuned for CI -- 4 attempts don't reliably outlast a sustained rate-limit window, and by the time the outer wrapper retries, the burst of parallel requests that likely triggered the 429 in the first place repeats. - cuDF and cuVS already carry a fix for this exact problem in their own Java/Maven builds: a project-level `.mvn/maven.config`, auto-applied to every `mvn` invocation with no wrapper script needed, that caps concurrent downloads and adds a real backoff inside Maven's own transport-layer retry handler. ## Fix Add `java/cuopt/.mvn/maven.config`, matching cuDF's (`java/.mvn/maven.config`) and cuVS's (`java/cuvs-java/.mvn/maven.config`) content exactly: ``` -e -B -Daether.connector.basic.downstreamThreads=1 -Daether.transport.http.retryHandler.count=5 -Daether.transport.http.retryHandler.interval=10000 -Dmaven.wagon.http.retryHandler.count=5 ``` - `aether.connector.basic.downstreamThreads=1` caps Maven's own concurrent download threads, reducing the burst of parallel requests against Maven Central that likely triggers the rate-limiting in the first place. - `aether.transport.http.retryHandler.interval=10000` adds a real 10s backoff inside Maven's own resolver, at the **transport** layer. `cuopt_mvn`'s existing `-D` flags (`java/cuopt/scripts/maven.sh`) target the **connector**-layer retry handler (`aether.connector.http.retryHandler.*`), which recent Maven resolver versions may no longer consult now that retry logic lives at the transport layer -- this adds the layer that actually gets read. Verified via: ``` mvn org.apache.maven.plugins:maven-help-plugin:3.4.0:evaluate \ -Dexpression=aether.transport.http.retryHandler.interval -q -DforceStdout # -> 10000 ``` `cuopt_mvn`'s outer shell-level retry loop is left in place as a second layer -- it's still useful for failures Maven's own retry can't cover (network drops mid-request, etc). ## Test plan - [x] Confirmed the property resolves correctly via `maven-help-plugin:evaluate` (see above). - [x] Ran the full `packaged-jar-tests` Maven Java suite locally with this config present; passes cleanly. - [ ] `java-build` and `java-static-test` CI run clean without hitting Maven Central 429s. Split out of #1818, where this was found while investigating an unrelated flaky Surefire crash -- this fix is independently useful for the existing `java-build` job today, not specific to that PR's self-contained classifier JAR work. Authors: - Ramakrishna Prabhu (https://github.com/ramakrishnap-nv) Approvers: - Ishika Roy (https://github.com/Iroy30) - James Lamb (https://github.com/jameslamb) URL: #1835

ramakrishnap-nv requested a review from a team as a code owner August 28, 2026 13:33

ramakrishnap-nv requested a review from Iroy30 August 28, 2026 13:33

ramakrishnap-nv self-assigned this Aug 28, 2026

ramakrishnap-nv added bug Something isn't working non-breaking Introduces a non-breaking change labels Aug 28, 2026

Trim comment on the set -e guard

6ab04ab

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

ramakrishnap-nv mentioned this pull request Aug 28, 2026

Include java-build in the PR test summary #1824

Open

Iroy30 approved these changes Aug 28, 2026

View reviewed changes

rapids-bot Bot merged commit 05a208e into main Aug 28, 2026
34 checks passed

ramakrishnap-nv mentioned this pull request Sep 1, 2026

Tune Maven's own retry/concurrency to reduce Maven Central rate-limiting #1835

Merged

3 tasks

Navigation Menu

Uh oh!

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

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

ramakrishnap-nv commented Aug 28, 2026

Uh oh!

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

github-actions Bot commented Aug 28, 2026

Uh oh!

ramakrishnap-nv commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ramakrishnap-nv commented Aug 28, 2026

Summary

Test plan

Uh oh!

coderabbitai Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

github-actions Bot commented Aug 28, 2026

CI Test Summary

Uh oh!

ramakrishnap-nv commented Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Aug 28, 2026 •

edited

Loading