fix: make shutdown tests reliable on windows-latest#2241
Closed
triuzzi wants to merge 1 commit into
Closed
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
e13bfc2 to
f3d63b1
Compare
The shutdown tests flake on windows-latest, for two distinct reasons:
1. stdin-EOF: the graceful path tears down the Chrome process tree via
`taskkill /T`, far slower than POSIX teardown (~6.5s observed in CI vs
<500ms) and noisy with benign "could not be terminated" retries, so the flat
3000ms budget is exceeded ("stdin-EOF shutdown took 6451ms (budget 3000ms)").
Give Windows an 8000ms budget — above the real teardown but below the
production force-exit backstop in chrome-devtools-mcp-main.ts (an unref'd
`setTimeout(process.exit(0), 10000)`), so a genuine hang rescued by that
backstop (exiting ~10s) is still caught.
2. SIGTERM/SIGINT/SIGHUP: Windows has no POSIX signals — Node maps
child.kill(signal) to TerminateProcess, which hard-kills the server without
running its handlers, so the graceful path never executes. Those three tests
would assert nothing and leak the Chrome process tree on Windows; skip them
there. stdin-EOF remains the cross-platform graceful-shutdown coverage.
Also reuse the shared IS_WINDOWS from src/daemon/utils.ts.
Related: ChromeDevTools#2137
f3d63b1 to
951a452
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
tests/shutdown.test.tsflakes onwindows-latest, for two distinct reasons.1. The stdin-EOF budget is too tight for Windows teardown. On a recent
mainrun:The graceful path tears down the Chrome process tree via
taskkill /T, far slower than POSIX teardown (~6.5s observed vs <500ms) and noisy with benign "could not be terminated" retries, so the flatSHUTDOWN_BUDGET_MS = 3000is exceeded.--test-concurrency=1(#2205) reduced overlap but doesn't change this budget.2. The signal tests don't exercise graceful shutdown on Windows.
child.kill('SIGTERM'|'SIGINT'|'SIGHUP')maps toTerminateProcesson Windows (no POSIX signals), so the server's handlers never run — those three tests hard-kill instantly, assert nothing about graceful shutdown, and leak the Chrome process tree.Fix
8000mson Windows (3000mselsewhere). 8000ms is above the real ~6.5s teardown but below the production force-exit backstop inchrome-devtools-mcp-main.ts(an unref'dsetTimeout(process.exit(0), 10000)), so a genuine hang rescued by that backstop (exiting ~10s) is still caught.child.kill(signal)can't trigger the graceful handlers there. stdin-EOF remains the cross-platform graceful-shutdown coverage.IS_WINDOWSfromsrc/daemon/utils.ts.Linux/macOS behavior is unchanged.
Related: #2137