ci: extend Maven setup and retry dependency downloads

[sunchao]

Which issue does this PR close?

No issue is closed. This follows up on #5422 and addresses the dependency-download failures seen on #5481. The separate libhdfs crash is covered by #5036.

Rationale for this change

#5422 added Maven distribution caching and bootstrap retries to the Java test action. Other Maven callers still bootstrap independently, and downloading Maven itself does not protect subsequent artifact downloads.

On #5481, the Delta build gate failed downloading a Maven plugin dependency with Connection reset, while a Spark 4.1 SQL shard failed resolving a Guava dependency with HTTP 502 before its tests started.

What changes are included in this PR?

• Extract the existing Maven bootstrap/cache setup into a shared action and use it across CI. Preserve the existing cache keys and macOS cache exclusion, and configure bounded Maven Resolver retries for individual HTTP transfers without rerunning Maven goals.
• Select the actual Maven wrapper distribution directory for both cache operations. Hosted runners no longer traverse /root, which caused cache saving to fail with EACCES; root containers keep their existing distribution location. The changed cache path list causes one initial cache miss.
• Add bounded retries for dependency-only commands. Retry recognized transient download errors, preserve failure exit codes, and leave compilation, tests, permanent errors, and cancellation fatal.
• Resolve Spark test dependencies before compilation, restore SBT/Coursier caches, and save successful downloads before later build or test failures. Only the full build job can publish the shared cache, so a shard cannot populate it with partial dependencies.
• Preserve Delta gate Maven/Cargo logs, propagate effective-POM failures, and upload the logs when the gate fails.
• Add regression checks to preflight and update change detection for the shared action and scripts.

How are these changes tested?

• 26 regression tests passed, including real Maven 3.9.6 requests against a local HTTP server for transient 429/502, exhausted 503 retries, and permanent 404 failures. Additional cases cover mixed transient/permanent failures (including long failure summaries), retry limits, exit codes, interrupted backoff, dependency-only Spark tasks, and Delta compilation failures and log retention.
• Six real-Java cache-path tests cover host and container homes, wrapper-home overrides, JVM selection, option precedence, and invalid options. The computed path also matches the actual local Maven wrapper installation.
• The updated hosted preflight passed on JDK 11, ran all 24 regression tests, and successfully saved the Maven distribution cache under /home/runner/.m2/wrapper/dists without EACCES.
• Workflow and composite-action validation with actionlint, YAML parsing, and Bash syntax checks passed.
• Existing suite/benchmark consistency checks and Apache RAT passed.

The real Maven integration checks ran locally on JDK 21 and in preflight on JDK 11. Full Spark/JVM/native coverage is provided by the remaining PR CI matrix. This PR does not change the native crash fix in #5036.

[comphead]

should we just try

export MAVEN_OPTS="-Dmaven.wagon.http.retryHandler.count=5 -Dhttp.keepAlive=false"

?

[andygrove]

Note on this review: this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.

Thanks for chasing this. Transient dependency-download failures are a real tax on everyone's PRs and the two linked failures make the case clearly. Factoring the Maven bootstrap into a shared composite action is overdue, and the cache-path.sh fix for EACCES on hosted runners is a good catch: globbing /root from a non-root job was always going to bite eventually.

I do have concerns, mostly about scope and about one cache key.

This is several independent changes in one PR

As far as I can tell there are at least four separable pieces here: the shared setup-maven action plus cache-path fix, the retry-download.sh wrapper, the SBT dependency pre-resolution and its new cache, and the Delta gate log preservation. CI changes are the hardest kind to review because most of the behavior only shows up in production, and if any one of these regresses, a combined PR means reverting all four. Would you be willing to split at least the SBT pre-resolution out? That one has the most new moving parts and the least connection to the download-failure problem in the title.

The SBT cache key mixes Comet and Spark poms

Restore SBT dependency cache keys on hashFiles('**/pom.xml', '!**/target/**', 'apache-spark/project/build.properties', 'apache-spark/project/*.sbt', 'apache-spark/project/*.scala'). The **/pom.xml glob is evaluated against the whole workspace, which at that point includes the cloned and patched apache-spark checkout. The saving job clones Spark itself. The test shards pass skip-spark-clone: true and get apache-spark/ from a pre-staged artifact.

If those two paths produce even slightly different file sets under apache-spark, the hash differs and the restore falls through to the restore-keys prefix. That still works, but it means the exact-key path never hits and you get whatever the most recent prefix match happens to be. Have you confirmed the primary key actually matches between the saving job and a restoring shard? If not, it would be more predictable to key on the Spark version plus the specific SBT build files and drop the wide pom glob, since Comet's own poms do not determine Spark's SBT dependency graph.

setup-maven is not idempotent in MAVEN_OPTS

The Configure Maven transfer retries step appends the aether.connector.http.retryHandler.* properties to whatever MAVEN_OPTS already holds and writes the result to $GITHUB_ENV. If a job ever ends up calling setup-maven twice, which is easy to do now that it is a shared action invoked from several places, the properties accumulate. It is harmless today because later -D wins, but it grows the command line and makes the logs confusing. Could the step skip appending when the property is already present?

cache-path.sh word-splits MAVEN_OPTS

read -r -a java_arguments <<< "$java_options" splits on whitespace with no quote handling. If anyone ever puts a quoted value containing a space into MAVEN_OPTS, for example an -XX:ErrorFile under a path with a space, the java -XshowSettings probe fails and the whole action errors out at a point that has nothing to do with the actual problem. Since only a handful of properties are needed to make the probe representative, would it be simpler to run the probe with a fixed minimal set of options rather than replaying whatever MAVEN_OPTS contains?

The transient-error regex matches anywhere in the log

retry-download.sh greps the entire combined output for the transient patterns. For a long sbt update over Spark, the log is large and includes artifact names and URLs. A permanent failure that happens to occur in the same run as an earlier recovered transient message would be retried three more times before failing, turning a fast failure into a slow one. Is it worth restricting the match to the tail of the log, or to lines that also indicate the command's own failure? The current behavior is safe in the sense that it never turns a failure into a pass, but the cost is real when it goes wrong.

One question on the preflight tests

test-download-retry.py and test-delta-gate.py add 600 lines that now run on every preflight. How long do they take? The preflight job is on the critical path for every PR, and 24 regression cases including real Maven requests against a local HTTP server sounds like it could be a minute or more. If it is significant, gating them on changes to dev/ci/** and the actions directory would keep the common case fast.

[sunchao]

Updated in 093db4c78.

Addressed the mixed-log retry bug: explicit permanent HTTP/artifact/build errors now stop the attempt even when an earlier line reports a connection reset. The new regressions fail on the previous helper and verify one invocation, the original exit status, and no backoff; a long-summary case prevents a fixed-tail workaround. The helper remains restricted to dependency acquisition, and Maven goals/Spark compilation and tests are not rerun.

All 26 download/cache/Delta checks pass locally, including Maven 3.9.6 against the loopback HTTP server. These are local results for the new commit; the previous hosted preflight ran the original 24 checks in about 36 seconds (29.105 seconds for download/cache tests and 4.942 seconds for Delta checks).

For the other questions:

• Maven 3.9.6 defaults to Resolver native HTTP, so Wagon retry settings do not replace the aether.connector.http.retryHandler settings here. The dependency-only SBT updates directly address the Spark download 502; they do not retry compilation or tests.
• The builder saved and the catalyst shard restored the same full Linux-X64-spark-sbt-v1-4.1.3-c4e071c238e732a97e0117554d9b25b46a1fdef8270b281b8c3eba49a4064eb8 key. Spark POMs remain in the key because SBT consumes them.
• Every current job calls setup-maven once. Repeating the same constant options would not alter their meaning; skipping already-present options could retain an incompatible caller policy.
• The quoted-space MAVEN_OPTS example fails identically in the unchanged mvnw launcher, so it is not a cache-probe regression. The probe must honor supported home overrides to select the same distribution directory as the wrapper; MAVEN_USER_HOME paths containing spaces remain tested.

I have kept the related download changes together: SBT pre-resolution addresses the observed dependency failure, and Delta logging retains the failing command's diagnostics. No build/test retry scope was added.

[andygrove]

The fix in 093db4c for the mixed-log retry bug addresses the concern I raised about a permanent failure getting retried just because an earlier line in the same log mentioned a transient one. The cache-key match and the setup-maven idempotency question both check out against the evidence in the thread, so I won't re-raise those.

This does overlap with #5782 in one concrete spot. Both PRs set aether.connector.http.retryHandler.count and aether.connector.http.retryHandler.serviceUnavailable, to different values, through different mechanisms. #5782's .mvn/maven.config applies those properties to every ./mvnw invocation, CI or local, with count 6 and service-unavailable codes 408,429,500,502,503,504. This PR's setup-maven/action.yaml sets count 3 and 429,500,502,503,504 through MAVEN_OPTS, which only takes effect where a job calls setup-maven. Landing both as written leaves two sources of truth for the same keys. .mvn/maven.config is the smaller change and the one that also helps contributors building locally, so I'd land #5782 first and have this PR drop its count and serviceUnavailable lines in favor of it, keeping the interval/intervalMax backoff pacing that .mvn/maven.config doesn't set.

Separately from #5782, this branch no longer merges cleanly against main. git merge-tree shows real conflicts in .github/workflows/ci.yml and .github/workflows/pyarrow_udf_test.yml. #5762 inserted a new preflight step at the same line this PR inserts Check CI download handling. #5561 inserted a new path filter and matrix entry at the same line this PR inserts .github/actions/setup-maven/**. More useful than the conflicts themselves: the celeborn-reflection-compatibility job that #5604 added to pr_build_linux.yml sets up its toolchain through setup-builder and then calls ./mvnw directly, bypassing java-test, rust-test, and setup-spark-builder, the three composite actions that now carry the Setup Maven step. Once this rebases, that job will still have neither the wrapper bootstrap retry nor the transfer-retry MAVEN_OPTS this PR is trying to make universal, so it's worth giving it a Setup Maven step in the same pass.

None of the new setup-maven action, retry-download.sh, or the SBT pre-resolution step gets a mention in .github/workflows/README.md. That matters more here because #5782 is adding a "Retrying flaky network operations" section to that same README describing ./.github/actions/java-test's bootstrap retry, which this PR moves into setup-maven. Whichever of the two lands second should update that section so it still matches where the mechanism actually lives.

[andygrove]

Triage note: #5881 is doing the same thing — bootstrapping Maven from the setup actions so every mvnw job survives a 403 or 429 from repo.maven.apache.org. It touches four of the same files: .github/actions/java-test/action.yaml, ci.yml, pr_build_linux.yml and dev/ci/compute-changes.py.

That one is mine and it is the newer of the two, so I plainly missed this when I opened it. This PR also retries the artifact downloads rather than only the distribution, which #5881 does not do at all. Would you rather take the combined change here, or shall I rebase #5881 onto whatever survives?

[andygrove]

Triage note: #5881 is doing the same thing — bootstrapping Maven from the setup actions so every mvnw job survives a 403 or 429 from repo.maven.apache.org. It touches four of the same files: .github/actions/java-test/action.yaml, ci.yml, pr_build_linux.yml and dev/ci/compute-changes.py.

That one is mine and it is the newer of the two, so I plainly missed this when I opened it. This PR also retries the artifact downloads rather than only the distribution, which #5881 does not do at all. Would you rather take the combined change here, or shall I rebase #5881 onto whatever survives?

@sunchao I went ahead and merged #5881 since it was green in CI and already approved. Maybe you could rebase this one after that merges?

[comphead]

do we still see network issues?