Skip to content

[ci] Add surefire fork timeouts to prevent CI hangs #6186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
joewiz wants to merge 2 commits into from
Closed

[ci] Add surefire fork timeouts to prevent CI hangs #6186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5de7278
[ci] Add surefire fork timeouts to prevent CI hangs
joewiz Mar 26, 2026
fda23d2
[ci] Add wagon timeouts to Maven Build; run license check offline
joewiz Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions .github/workflows/ci-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:
distribution: liberica
java-version: ${{ env.DEV_JDK }}
- uses: ./.github/actions/maven-cache
- run: mvn -V -B --no-transfer-progress license:check
- run: mvn -V -B --no-transfer-progress --offline license:check
timeout-minutes: 10
dependencies:
name: Dependency checks
Expand Down Expand Up @@ -88,14 +88,19 @@ jobs:
timeout-minutes: 30
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
MAVEN_OPTS: >-
-DtrimStackTrace=false
Copy link
Copy Markdown
Contributor

@duncdrum duncdrum Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like centralising these options, but we are doing it only half-heartedly e.g. -DtrimStackTrace=false is appears in the env and on every invocation. We should make this DRY. There are more flags similarly repeated

Copy link
Copy Markdown
Member Author

@joewiz joewiz Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[This response was co-authored with Claude Code. -Joe]

Agreed — this is a real smell and worth cleaning up properly. I'll address this as part of reworking the transport question; once we settle on native vs wagon the flag set will change anyway, and that's a good moment to centralize everything.

-D'maven.resolver.transport=wagon'
Copy link
Copy Markdown
Contributor

@duncdrum duncdrum Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want wagon at all? Its no longer the default https://maven.apache.org/guides/mini/guide-resolver-transport.html for a reason

Copy link
Copy Markdown
Member Author

@joewiz joewiz Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[This response was co-authored with Claude Code. -Joe]

Good question, and the git history has an answer: -Dmaven.resolver.transport=wagon was added by Dannes on 2023-03-02 with commit message "testing" — almost certainly a temporary workaround for instability in Maven 3.9.0's native transport when it first became the default. It was never revisited. mvnd 1.0.3 bundles Maven 3.9.11, so whatever prompted the workaround is long fixed.

I'd like to try removing it (and the wagon-specific timeout flags) and replacing with Aether native timeout flags (aether.connector.basic.connectTimeout, aether.connector.basic.requestTimeout). Will open a test branch and report back.

-D'maven.wagon.http.connectionTimeout=10000'
-D'maven.wagon.http.readTimeout=30000'
run: ${{ steps.install-mvnd.outputs.mvnd-dir }}/mvnd -V -B --no-transfer-progress -T 1C compile test-compile -DtrimStackTrace=false -D'dependency-check.skip' -D'license.skip' --projects '!exist-installer'
- name: Maven Unit Tests
if: matrix.test-type == 'unit'
timeout-minutes: 45
run: ${{ steps.install-mvnd.outputs.mvnd-dir }}/mvnd -V -B --no-transfer-progress test -DtrimStackTrace=false -D'dependency-check.skip' -D'license.skip' -D'mvnd.maxLostKeepAlive=6000'
- name: Maven Integration Tests
if: matrix.test-type == 'integration'
timeout-minutes: 45
timeout-minutes: 30
Copy link
Copy Markdown
Contributor

@duncdrum duncdrum Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen 37min successful runs on CI. I would suggest to be more generous with the CI timeout until we can establish a new baseline of successful CI runs, and then lower the workflow timeouts accordingly.

Copy link
Copy Markdown
Member Author

@joewiz joewiz Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[This response was co-authored with Claude Code. -Joe]

Good to know — reverting to 45 minutes. The 30-minute reduction was premature given you've seen legitimate 37-minute runs.

run: ${{ steps.install-mvnd.outputs.mvnd-dir }}/mvnd -V -B --no-transfer-progress verify -DskipUnitTests=true -DtrimStackTrace=false -D'dependency-check.skip' -D'license.skip' -D'mvnd.maxLostKeepAlive=6000'
- name: Javadoc (ubuntu unit only)
if: matrix.os == 'ubuntu-latest' && matrix.test-type == 'unit'
Expand Down
8 changes: 8 additions & 0 deletions exist-parent/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,10 @@
However it can make it hard to diagnose problems if tests leak state; If you experience
such a problem you may want to set it to `false` whilst debugging -->
<reuseForks>true</reuseForks>
<!-- Kill forked JVMs that hang (e.g. DeadlockIT, MoveResourceTest).
10 minutes is generous; clean runs complete in ~3.5 minutes. -->
<forkedProcessTimeoutInSeconds>600</forkedProcessTimeoutInSeconds>
Copy link
Copy Markdown
Contributor

@duncdrum duncdrum Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea with #6045 was to move slow tests into the IT phase. Unit test should be faster, so I would expect the timeouts here to be lower compared to failsafe tests. If there are slow tests left in surefire, I would rather move them to IT.
Does surefire even fork a jvm? I thought this was a failsafe thing?

Copy link
Copy Markdown
Member Author

@joewiz joewiz Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[This response was co-authored with Claude Code. -Joe]

Surefire does fork here — forkCount=2C with reuseForks=true in exist-parent/pom.xml, so forkedProcessTimeoutInSeconds is applicable. That said, your point about lower timeouts for unit tests is well taken, and moving slow stragglers to the IT phase (#6045) is the right long-term answer. For now I'll remove the forkedProcessTimeoutInSeconds from surefire and keep it only in failsafe, where it has the clearest justification.

<forkedProcessExitTimeoutInSeconds>60</forkedProcessExitTimeoutInSeconds>
<argLine>@{jacocoArgLine} --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.ref=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED -Dfile.encoding=${project.build.sourceEncoding}</argLine>
<systemPropertyVariables>
<user.country>UK</user.country>
Expand All @@ -863,6 +867,10 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-failsafe-plugin</artifactId>
<version>3.5.5</version>
<configuration>
<forkedProcessTimeoutInSeconds>600</forkedProcessTimeoutInSeconds>
<forkedProcessExitTimeoutInSeconds>60</forkedProcessExitTimeoutInSeconds>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
Expand Down
Loading