fix(e2e): use taskkill to kill processes on windows - #1760
Conversation
commit: |
📝 WalkthroughWalkthroughThe server shutdown path now uses Windows Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/e2e/server.ts`:
- Around line 1-3: Update the taskkill path used by stopServer to avoid
unbounded xSync blocking: pass a short tinyexec timeout or replace it with
asynchronous execution plus an explicit timeout, and treat timeout failures as
non-fatal so stopServer can continue to ctx.nuxt.close(), browser cleanup, and
teardown hooks.
In `@src/e2e/setup/index.ts`:
- Around line 51-68: Update the teardown flow around `deadline`, `remaining`,
and `withTimeout` so the shared `ctx.options.teardownTimeout` is a strict single
deadline: return 0 when expired, route `stopServer()` through the same timeout
limiter, and make `withTimeout` accept a lazy operation so no close or teardown
action starts after the budget is exhausted. Preserve the existing teardown
order and warning behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b9462957-72db-4482-9144-50f859d9b5f3
📒 Files selected for processing (2)
src/e2e/server.tssrc/e2e/setup/index.ts
| import { x, xSync } from 'tinyexec' | ||
| import { getRandomPort, waitForPort } from 'get-port-please' | ||
| import { isWindows } from 'std-env' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For tinyexec 1.2.4, how can xorxSyncexecutetaskkillwith a timeout, and doesxSyncforwardnodeOptions.timeoutto Node.jschild_process.spawnSync?
💡 Result:
In tinyexec 1.2.4, you can execute taskkill with a timeout using xSync by passing a timeout property (in milliseconds) within the options object [1]. Example: xSync('taskkill', ['/F', '/IM', 'process.exe'], { timeout: 5000 }); Regarding your second question, yes, xSync forwards nodeOptions to the underlying Node.js process execution function. While the synchronous API does not support features like AbortSignal, persist, or piping, other options including timeout and nodeOptions are supported and function in the same way as they do in the asynchronous API [1]. Because xSync ultimately utilizes Node.js's synchronous child process methods (such as spawnSync), the timeout and any provided nodeOptions are passed through to the underlying Node.js implementation [1][2].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate server.ts =="
fd -a 'server\.ts$' . || true
echo "== package tinyexec versions =="
if [ -f package.json ]; then
node - <<'JS'
const fs = require('fs');
const pkg = JSON.parse(fs.readFileSync('package.json','utf8'));
console.log('package.json:', JSON.stringify({
dependencies: pkg.dependencies,
devDependencies: pkg.devDependencies
}, null, 2));
JS
fi
if [ -f pnpm-lock.yaml ]; then
rg -n "tinyexec|get-port-please|std-env" pnpm-lock.yaml | head -80 || true
elif [ -f package-lock.json ]; then
rg -n "tinyexec|get-port-please|std-env" package-lock.json | head -80 || true
elif [ -f yarn.lock ]; then
rg -n "tinyexec|get-port-please|std-env" yarn.lock | head -80 || true
fi
echo "== inspect src/e2e/server.ts relevant lines =="
wc -l src/e2e/server.ts
sed -n '1,240p' src/e2e/server.ts
echo "== inspect setup teardown references =="
sed -n '1,120p' src/e2e/setup/index.tsRepository: nuxt/test-utils
Length of output: 14905
🌐 Web query:
tinyexec 1.2.4 README timeout option taskkill timeout nodeOptions.timeout xSync
💡 Result:
In tinyexec 1.2.4, the timeout option is used to specify a duration in milliseconds after which a process will be forcibly killed [1][2]. This functionality is available for both the asynchronous x and synchronous xSync APIs [1][2]. Regarding your specific keywords: timeout: This option is a property within the options object passed to x or xSync [1][3]. When set, the library ensures the process is terminated after the specified time [1][3]. Under the hood, this integrates with Node.js child process mechanisms to manage the timeout [4]. taskkill: The library does not explicitly mention taskkill in its primary documentation [1][5][2]. It manages process termination via Node.js's built-in child process signals (defaulting to SIGTERM) when the timeout is reached or the kill method is called [1][5]. nodeOptions.timeout: While nodeOptions allows you to pass custom options to the underlying Node.js spawn function, the library's official documentation defines a top-level timeout option [1][6]. Users are encouraged to use the explicit timeout option provided by tinyexec for consistent behavior [1][2]. xSync: The xSync function supports the same core options as the asynchronous x function, including timeout, throwOnError, and nodeOptions [2][3]. Although xSync blocks the event loop and does not support features like signal, persist, or piping, the timeout mechanism remains functional for synchronous execution [1][2].
Citations:
- 1: https://unpkg.com/tinyexec@1.2.4/README.md
- 2: https://github.com/tinylibs/tinyexec
- 3: https://context7.com/tinylibs/tinyexec/llms.txt
- 4: tinylibs/tinyexec@634f900
- 5: https://github.com/tinylibs/tinyexec/
- 6: https://www.npmjs.com/package/tinyexec
Bound the taskkill invocation to a teardown timeout.
xSync blocks the event loop until taskkill exits, so the stopServer() Promise.race cannot start until the call returns. If taskkill stalls, teardown cannot continue to ctx.nuxt.close(), browser close, or teardown hooks in src/e2e/setup/index.ts. Pass a small tinyexec timeout, or run the command asynchronously with an explicit timeout; treat a timeout as a non-fatal teardown result.
[st稳定性_and_availability]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e/server.ts` around lines 1 - 3, Update the taskkill path used by
stopServer to avoid unbounded xSync blocking: pass a short tinyexec timeout or
replace it with asynchronous execution plus an explicit timeout, and treat
timeout failures as non-fatal so stopServer can continue to ctx.nuxt.close(),
browser cleanup, and teardown hooks.
| // Every step is bounded against a single shared budget so that a shutdown | ||
| // that never settles degrades to a warning instead of failing the run. | ||
| const deadline = Date.now() + ctx.options.teardownTimeout | ||
| const remaining = () => Math.max(1_000, deadline - Date.now()) | ||
|
|
||
| if (ctx.serverProcess) { | ||
| setTestContext(ctx) | ||
| await stopServer() | ||
| setTestContext(undefined) | ||
| } | ||
| if (ctx.nuxt && ctx.nuxt.options.dev) { | ||
| await ctx.nuxt.close() | ||
| await withTimeout('closing the Nuxt instance', remaining(), ctx.nuxt.close()) | ||
| } | ||
| if (ctx.browser) { | ||
| await ctx.browser.close() | ||
| await withTimeout('closing the browser', remaining(), ctx.browser.close()) | ||
| } | ||
| // clear side effects | ||
| await Promise.all(!ctx.teardown ? [] : ctx.teardown.map(fn => fn())) | ||
| await withTimeout('running teardown hooks', remaining(), Promise.all(!ctx.teardown ? [] : ctx.teardown.map(fn => fn()))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Enforce teardownTimeout as one deadline.
remaining() returns 1,000ms after deadline. Each later teardown step can therefore wait another second. stopServer() is also awaited outside withTimeout, even though the deadline starts before it.
The total afterAll duration can exceed ctx.options.teardownTimeout by the server shutdown time and by one second per later step. Use 0 as the expired budget, and apply the same limiter to server shutdown. Make withTimeout accept a lazy operation so an expired deadline does not start another close operation.
Proposed direction
-function withTimeout<T>(label: string, ms: number, promise: Promise<T>): Promise<T | undefined> {
+function withTimeout<T>(label: string, ms: number, operation: () => Promise<T>): Promise<T | undefined> {
+ if (ms <= 0) {
+ console.warn(`[`@nuxt/test-utils`] Timed out ${label} during teardown. Continuing; some processes or file handles may still be held.`)
+ return Promise.resolve(undefined)
+ }
// ...
- const guarded = promise.catch(/* ... */)
+ const guarded = Promise.resolve().then(operation).catch(/* ... */)
}
-const remaining = () => Math.max(1_000, deadline - Date.now())
+const remaining = () => Math.max(0, deadline - Date.now())
-await stopServer()
+await withTimeout('stopping the test server', remaining(), () => stopServer())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/e2e/setup/index.ts` around lines 51 - 68, Update the teardown flow around
`deadline`, `remaining`, and `withTimeout` so the shared
`ctx.options.teardownTimeout` is a strict single deadline: return 0 when
expired, route `stopServer()` through the same timeout limiter, and make
`withTimeout` accept a lazy operation so no close or teardown action starts
after the budget is exhausted. Preserve the existing teardown order and warning
behavior.
2 participants
🔗 Linked issue
📚 Description
this fixes a couple of issues when tearing down a fixture on windows:
properly kill the whole task tree
timeout the teardown so it doesn't fail the test suite