Cleanup Gradle build daemon after Launcher tests#11499
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 136bb6ee87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env.put("GRADLE_USER_HOME", gradleUserHome.toString()); | ||
| env.put("GRADLE_OPTS", ""); | ||
| ShellCommandExecutor shellCommandExecutor = | ||
| new ShellCommandExecutor(projectFolder.toFile(), GRADLE_STOP_TIMEOUT_MILLIS, env); |
There was a problem hiding this comment.
Use an existing project directory for daemon shutdown
Because projectFolder is an instance @TempDir from AbstractGradleTest, JUnit scopes it to each parameterized invocation and cleans it up when that invocation finishes; by the time this @AfterAll runs, the last projectFolder has already been deleted. In that state ShellCommandExecutor starts ./gradlew --stop with a non-existent working directory, the exception is swallowed by the best-effort catch, and the shared static gradleUserHome is still left for JUnit to delete while Gradle daemons may hold files open.
Useful? React with 👍 / 👎.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
- Let's test on CI with [NON_DEFAULT_JVMS=true].
- Change PR description
AfterAll->AfrerEach.
|
| protected List<String> buildJvmArguments( | ||
| String mockBackendIntakeUrl, String serviceName, Map<String, String> additionalArgs) { | ||
| List<String> arguments = new ArrayList<>(Arrays.asList("-Xms256m", "-Xmx256m")); | ||
| List<String> arguments = new ArrayList<>(Arrays.asList("-Xms512m", "-Xmx512m")); |
There was a problem hiding this comment.
nit: probably we may try to give a chance to use lower limits: "-Xms256m", "-Xmx512m" WDYT?
[ci: NON_DEFAULT_JVMS]
What Does This Do
Makes the Gradle smoke tests robust against intermittent CI failures, with three related changes:
GradleLauncherSmokeTestteardown flake: adds an@AfterEachhook that runs./gradlew --stopto stop the Gradle build daemon before JUnit deletes the static@TempDir gradleUserHome.GradleDaemonSmokeTestteardown flake: adds an@AfterAllhook that callsDefaultGradleConnector.close()to stop the TestKit daemons before JUnit deletes the static@TempDir testKitFolder.succeed-gradle-plugin-testOutOfMemoryError: bumps the shared smoke-test build/daemon heap from-Xmx256mto-Xmx512minCiVisibilitySmokeTest.Motivation
GradleLauncherSmokeTest
GradleLauncherSmokeTestwas flaky in CI. The test itself always passed, but teardown intermittently failed with:Even though the launcher runs with
--no-daemon, Gradle still spawns a single-use build daemon that writes into$GRADLE_USER_HOME/daemon/<version>/. When that process hasn't fully released its file handles by the time JUnit's static@TempDircleanup runs, the recursive delete races against it and fails withDirectoryNotEmptyException, surfacing as a class-level failure.Explicitly stopping the daemon in
@AfterEach(which runs before the static@TempDircleanup) makes the deletion deterministic.GradleDaemonSmokeTest
The TestKit-based
GradleDaemonSmokeTestsuffers from the same class of teardown failure:TestKit runs every build in a daemon rooted under
<testKitFolder>/test-kit-daemonwith a 120s idle timeout. At class teardown a daemon is often still alive and still holding file handles on itscaches/<version>directory, so JUnit's recursive delete of the static@TempDir testKitFolderfails.OutOfMemoryError in succeed-gradle-plugin-test
The
test-succeed-gradle-plugin-testproject appliesjava-gradle-plugin, which putsgradleApi()on its compile classpath. This could overflow the 256m daemon heap, making the build die with:Additional Notes
test-environment-trigger: skip
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-levelJira ticket: [PROJ-IDENT]