Skip to content

Fix flaky RunningApplicationProcessSpec by widening the stop() timeout budget #16032

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

Merged
jdaugherty merged 3 commits into from
Aug 16, 2026
+15 −5
Merged

Fix flaky RunningApplicationProcessSpec by widening the stop() timeout budget #16032

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e71d9fa
Fix flaky RunningApplicationProcessSpec stop timeout
borinquenkid Jul 21, 2026
2d617df
Fix the actual reaper-thread flake in RunningApplicationProcessSpec
borinquenkid Jul 27, 2026
3811fce
Poll ProcessHandle.isAlive(), not Process.isAlive(), in the reaper-la…
borinquenkid Aug 1, 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
4 changes: 2 additions & 2 deletions grails-shell-cli/src/test/groovy/org/grails/cli/gradle/RunningApplicationProcessSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,8 @@ class RunningApplicationProcessSpec extends Specification {
expect:
RunningApplicationProcess.isRunning(pidFile)

when:
def result = RunningApplicationProcess.stop(pidFile, 15000)
when: "a generous timeout budget gives headroom for reaper-notification lag on a contended CI runner"

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The block description explains why the number is large rather than what the stimulus is, so the Spock report ends up reading when: a generous timeout budget gives headroom for reaper-notification lag on a contended CI runner for a step whose stimulus is simply "the application is stopped". It also undersells the change: 30000 is not an arbitrary generous value, it is exactly what the shipped stop-app passes (grails-profiles/base/commands/stop-app.groovy:31). Saying that makes this an alignment with production instead of a number we grew until the test stopped failing.

when: "the application is stopped"
// Same budget the shipped stop-app command passes (grails-profiles/base/commands/stop-app.groovy)
def result = RunningApplicationProcess.stop(pidFile, 30_000)

Also 30_000 for consistency with the 3_600_000L literals already in this file. If you want to go further, hoisting the value onto RunningApplicationProcess (static final long DEFAULT_STOP_TIMEOUT_MILLIS = 30_000L) and using it from both stop-app.groovy and here would leave one source of truth instead of two copies of the same magic number - fine to skip if you would rather keep the diff minimal.

@borinquenkid borinquenkid Aug 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — the when: label now reads "the application is stopped", and there's a comment noting 30_000 matches the value stop-app.groovy:31 passes in production, rather than framing it as an arbitrary generous number. Kept the underscore style consistent with the 3_600_000L literals already in the file. Left the DEFAULT_STOP_TIMEOUT_MILLIS hoist out per your "fine to skip" note, to keep the diff minimal.

def result = RunningApplicationProcess.stop(pidFile, 30000)

@jdaugherty jdaugherty Jul 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the part I would like confirmed before merge.

stop() can only return STILL_RUNNING if awaitExit() returns false twice: once after the timeoutMillis wait on onExit(), and again 5s after destroyForcibly() (note Math.min(timeoutMillis, 5000L) - the second budget does not move with this change, so the effective total goes 20s -> 35s). Both of those returns fall through to !process.isAlive(), a direct OS liveness check. For the target here - sleep 60 / ping -n 60 127.0.0.1, hit with destroy() and then destroyForcibly() - the OS still reporting it alive ~20s later is hard to credit. NOT_RUNNING is ruled out by the expect: isRunning(pidFile) immediately above. So I do not think the 15s budget is what produced the failure.

The candidate I would check first is the tail condition seven lines down, process.waitFor(10, TimeUnit.SECONDS). It returns false - a failing Spock condition - when the test JVM's own bookkeeping for its child has not caught up within 10s, which is precisely the reaper-notification lag the description blames, and this PR leaves its 10s budget untouched. That tail is also what 6c76333/a13c38c3fd were fixing. If that is the condition failing on CI, this change will not stop the flake and the fix belongs there instead - an unbounded process.waitFor(), or a PollingConditions wait on !process.isAlive().

Worth ruling out as well: stop() discards the boolean returned by destroy()/destroyForcibly(), so a signal that never landed is indistinguishable from a slow shutdown. Not something to fix in this PR, but it is the other way STILL_RUNNING happens without any timeout being too short.

The Spock output from one flaky run settles which of these it is, since it prints the rendered condition and the actual StopResult.

@borinquenkid borinquenkid Aug 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You were right — the 15s→30s stop() budget wasn't the fix. Replaced the tail process.waitFor(10, TimeUnit.SECONDS) with a PollingConditions wait on the OS-backed liveness check, same idea as your suggested alternatives.

One more round worth flagging: my first pass at that polled process.isAlive() (java.lang.Process), reasoning it was OS-backed like stop()'s own fallback. Checked the JDK 21 source to confirm and that's wrong — Process#isAlive() is !hasExited, set only when the same async ProcessHandleImpl reaper-thread completion that backs waitFor()/onExit() resolves. It's not a live query, so polling it would've just re-widened the same budget from 10s to 30s rather than removing the dependency you flagged. Fixed to poll process.toHandle().isAlive() instead, which delegates to ProcessHandleImpl's native isAlive0() — a genuine per-call OS check, matching what awaitExit()'s fallback actually relies on. That version is pushed now.


then:
result == RunningApplicationProcess.StopResult.STOPPED
Expand Down
Loading