Destroy process tree on exec abort by jjohannes · Pull Request #33596 · gradle/gradle

jjohannes · 2025-05-23T08:31:56Z

Fixes #7603
New attempt based on #18756

Note

❗ Most of the added lines in this PR is test and test setup code. There is effectively just one line of new productions code: process.descendants().forEach(java.lang.ProcessHandle::destroy);

Context

With Java 9+ Java offers APIs to solve this OS independently. With Gradle 9 (next release), the minimum Java version for running the Gradle daemon is 17. Therefore, a solution that only works for Java 9+ is reasonable.

If the ExecHandleRunner code (modified in this PR) runs in a worker process, the old behavior is kept and a message about that is logged on DEBUG level.

This PR is following up on a discussion on Slack with @oleg-nenashev and @jvandort.

Contributor Checklist

Review Contribution Guidelines.
Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team.
Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective.
Provide unit tests (under <subproject>/src/test) to verify logic.
~~[ ] Update User Guide, DSL Reference, and Javadoc for public-facing changes.~~
Ensure that tests pass sanity check: ./gradlew sanityCheck.
Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest.

Reviewing cheatsheet

Before merging the PR, comments starting with

❌ ❓must be fixed
🤔 💅 should be fixed
💭 may be fixed
🎉 celebrate happy things

Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>

ov7a · 2025-05-23T13:23:32Z

Thank you for your proposed contribution!

This PR has a valid DCO and tests. The relevant team for this area will confirm the design of the implementation choices.

big-guy · 2025-06-02T17:16:12Z

+        execHandle.waitForFinish().exitValue != 0
+    }
+
+    @Requires(UnitTestPreconditions.Jdk8OrEarlier)


Unit tests no longer run on anything below Java 17, so this will never run. We need an integration test to exercise this properly on Windows.

I added an integration test for the use case the feature solves. Unfortunately, it only runs on Unix, as I did not find a fixture for testing it on Windows. This also screams flakiness.

As this change has no Windows specific code on the Gradle side, I think is is fine though. And the code is tested on Windows in the unit test. I don't think in this case an integration test adds much. It's executing the same code path.

Regarding the test above I now removed: I am not sure how to get code to even execute on Java 8. So I did not any replacement.

If you think there should be additional tests, please advice what exactly we should add.

Fixed this and added integration tests for Windows

Thank you @asodja for picking this up. Much appreciated. 🙇

Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>

doctorpangloss · 2025-07-02T21:05:57Z

this should get merged

asodja · 2025-07-21T08:01:59Z

I will take it from here and take care that it gets in 9.1.0.

jvandort · 2025-07-22T14:32:00Z


 package org.gradle.integtests.fixtures

+


jvandort · 2025-07-22T14:34:25Z

+        return getProcessHandle(pid)
+    }
+
+    private static def getProcessHandle(Long pid) {


This should be marked with @Nullable

jvandort · 2025-07-22T14:41:50Z

+    private DaemonClientFixture client
+    private int daemonLogCheckpoint
+
+    @Requires(UnitTestPreconditions.Jdk9OrLater)


This should not be necessary. All integ tests run on 17+

doctorpangloss · 2025-07-23T00:35:00Z

i backported this on top of 8.14 since too many plugins don't support 9 yet, i hope you guys will too
works flawlessly on desktop windows, macos, linux and our windows and linux CI
it fixes so many pain points...

Co-authored-by: Justin <jvandort@gradle.com>

asodja · 2025-07-23T09:30:00Z

@doctorpangloss I am not sure if we will backport this, technically 8.x line is only in maintenance mode. We will discuss it internally

ljacomet · 2025-07-23T13:13:59Z

@doctorpangloss yes, this kind of fix would be a good value add to backport. ~~But unfortunately, it relies on Java 9+ APIs and Gradle 8.x still supports running on Java 8+.~~
So it is unlikely we will be able to backport this as-is. If you have further interest in this and believe there is an easy way to backport while respecting the compatibility, please open an issue describing your thoughts.

Well, replied too fast, the code here has the right safeguards against that.
I created:

[Backport] Gradle doesn't stop forked processes #34365

asodja · 2025-07-23T13:51:25Z

@jjohannes Thank you for your contribution!

What a mess. See: - [Terminating Gradle build does not execute finalizedBy task](gradle/gradle#13212) - [`finalizedBy` does not work when task fails and the target task depends on origin task](gradle/gradle#27707) - [Finalizer tasks are not run when the build is cancelled](gradle/gradle#28399) - [Gradle doesn't stop forked processes](gradle/gradle#7603) - [Stop forked exec and javaexec on build termination](gradle/gradle#18756) - [Destroy process tree on exec abort](gradle/gradle#33596)"

Backport of #33596 Fixes #34365

Allow use of Java 17 APIs in 'core'

f26317b

Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>

jjohannes requested a review from a team May 23, 2025 08:31

bot-gradle added from:contributor PR by an external contributor to-triage labels May 23, 2025

jjohannes force-pushed the destroy-process-tree-on-exec-abort branch 2 times, most recently from 3e889cc to 5c8ae37 Compare May 23, 2025 08:56

ExecHandleRunner destroys child processes

727131e

Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>

jjohannes force-pushed the destroy-process-tree-on-exec-abort branch from 5c8ae37 to 727131e Compare May 23, 2025 08:59

big-guy added the 👋 team-triage Issues that need to be triaged by a specific team label May 23, 2025

big-guy added the in:exec-tasks label May 23, 2025

ov7a removed the to-triage label May 23, 2025

big-guy reviewed Jun 2, 2025

View reviewed changes

jvandort reviewed Jun 2, 2025

View reviewed changes

Comment thread subprojects/core/build.gradle.kts Outdated

mlopatkin removed the 👋 team-triage Issues that need to be triaged by a specific team label Jun 10, 2025

jjohannes added 3 commits June 11, 2025 11:01

Format configuration code

363f2bb

Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>

Remove unit test that would never execute

2e8c027

Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>

Add integration test

6604463

Signed-off-by: Jendrik Johannes <jendrik.johannes@gmail.com>

jjohannes requested review from big-guy and jvandort June 11, 2025 09:11

big-guy added the 👋 team-triage Issues that need to be triaged by a specific team label Jun 24, 2025

doctorpangloss mentioned this pull request Jul 2, 2025

Ctrl+C to the gradle wrapper on macOS and Windows does not kill subprocesses started by Exec unexpectedly #34095

Closed

ljacomet mentioned this pull request Jul 16, 2025

Stop forked exec and javaexec on build termination #18756

Closed

13 tasks

github-actions Bot added the to-triage label Jul 17, 2025

bamboo added this to the 9.1.0 RC1 milestone Jul 17, 2025

ov7a removed the to-triage label Jul 17, 2025

asodja mentioned this pull request Jul 21, 2025

[Testing] Destroy process tree on exec abort for Java9+ #34305

Closed

asodja closed this Jul 21, 2025

alansemenov mentioned this pull request Jul 21, 2025

Termination of dev task won't kill npm processes enonic/starter-tsup#452

Open

asodja self-assigned this Jul 21, 2025

jvandort approved these changes Jul 22, 2025

View reviewed changes

asodja force-pushed the destroy-process-tree-on-exec-abort branch 2 times, most recently from 046d94a to 4631b1e Compare July 23, 2025 08:17

Polish DestroyForkedProcessesIntegrationTest and ProcessFixture

728fc34

Co-authored-by: Justin <jvandort@gradle.com>

asodja force-pushed the destroy-process-tree-on-exec-abort branch from 4631b1e to 728fc34 Compare July 23, 2025 09:18

gradle deleted a comment from asodja Jul 23, 2025

asodja added this pull request to the merge queue Jul 23, 2025

bot-gradle added this to the 9.1.0 RC1 milestone Jul 23, 2025

ljacomet mentioned this pull request Jul 23, 2025

[Backport] Gradle doesn't stop forked processes #34365

Closed

Merged via the queue into gradle:master with commit 7633dee Jul 23, 2025
28 checks passed

asodja mentioned this pull request Jul 23, 2025

Gradle doesn't stop forked processes #7603

Closed

ljacomet pushed a commit that referenced this pull request Jan 20, 2026

Destroy process tree on exec abort

450b68b

Backport of #33596 Fixes #34365

ljacomet mentioned this pull request Jan 20, 2026

Destroy process tree on exec abort #36345

Merged

ljacomet pushed a commit that referenced this pull request Jan 20, 2026

Destroy process tree on exec abort

c1d9888

Backport of #33596 Fixes #34365

ljacomet pushed a commit that referenced this pull request Jan 20, 2026

Destroy process tree on exec abort

03d3245

Backport of #33596 Fixes #34365

ljacomet pushed a commit that referenced this pull request Jan 21, 2026

Destroy process tree on exec abort

38f42ed

Backport of #33596 Fixes #34365

ljacomet pushed a commit that referenced this pull request Jan 21, 2026

Destroy process tree on exec abort

1805cf6

Backport of #33596 Fixes #34365

jjohannes deleted the destroy-process-tree-on-exec-abort branch March 27, 2026 07:37

Conversation

jjohannes commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Contributor Checklist

Reviewing cheatsheet

Uh oh!

ov7a commented May 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

doctorpangloss commented Jul 2, 2025

Uh oh!

asodja commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

doctorpangloss commented Jul 23, 2025

Uh oh!

asodja commented Jul 23, 2025

Uh oh!

ljacomet commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

asodja commented Jul 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jjohannes commented May 23, 2025 •

edited

Loading

asodja commented Jul 21, 2025 •

edited

Loading

ljacomet commented Jul 23, 2025 •

edited

Loading

asodja commented Jul 23, 2025 •

edited

Loading