fix(node): call close hooks on server shutdown - #4522
Conversation
Restore v2 behavior for the `node_server` and `node_cluster` presets: run runtime `close` hooks when srvx closes the server on SIGINT/SIGTERM. Resolves nitrojs#4502
|
@tarikermis is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughChangesNode shutdown hooks
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 1
🧹 Nitpick comments (1)
src/presets/node/runtime/node-cluster.ts (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate implementation comments.
src/presets/node/runtime/node-cluster.ts#L35-L35: Remove the comment.src/presets/node/runtime/node-server.ts#L29-L29: Remove the comment.As per coding guidelines, “Do not add comments explaining what the line does unless prompted.”
🤖 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/presets/node/runtime/node-cluster.ts` at line 35, Remove the duplicate implementation comments in src/presets/node/runtime/node-cluster.ts lines 35-35 and src/presets/node/runtime/node-server.ts lines 29-29, leaving the surrounding shutdown and close-hook logic unchanged.Source: Coding guidelines
🤖 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 `@test/presets/node.test.ts`:
- Around line 61-85: The spawned server in the test flow must always be
terminated, including when waitForPort, the close wait, or an assertion fails.
Wrap the startup, waiting, assertions, and close-hook handling around the child
created by execa in a try block, and move the SIGKILL cleanup into finally while
preserving the existing graceful SIGTERM and close-marker behavior.
---
Nitpick comments:
In `@src/presets/node/runtime/node-cluster.ts`:
- Line 35: Remove the duplicate implementation comments in
src/presets/node/runtime/node-cluster.ts lines 35-35 and
src/presets/node/runtime/node-server.ts lines 29-29, leaving the surrounding
shutdown and close-hook logic unchanged.
🪄 Autofix
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: 0bccb7ec-22c1-45b5-961b-e142b9c870ba
📒 Files selected for processing (4)
src/presets/node/runtime/node-cluster.tssrc/presets/node/runtime/node-server.tstest/fixture/server/plugins/close.tstest/presets/node.test.ts
| const child = execa(process.execPath, [entryPath], { env, extendEnv: false, reject: false }); | ||
|
|
||
| let output = ""; | ||
| child.stdout!.on("data", (data) => (output += data)); | ||
| child.stderr!.on("data", (data) => (output += data)); | ||
|
|
||
| await waitForPort(port, { delay: 1000, retries: 20, host: "127.0.0.1" }); | ||
|
|
||
| child.kill("SIGTERM"); | ||
| // Wait for the close hook marker or process exit (the fixture task scheduler | ||
| // can keep the event loop alive after the server closed) | ||
| await new Promise<void>((resolve) => { | ||
| const timeout = setTimeout(resolve, 10_000); | ||
| child.on("close", () => { | ||
| clearTimeout(timeout); | ||
| resolve(); | ||
| }); | ||
| child.stdout!.on("data", (data) => { | ||
| if (String(data).includes("[fixture] close hook called")) { | ||
| clearTimeout(timeout); | ||
| resolve(); | ||
| } | ||
| }); | ||
| }); | ||
| child.kill("SIGKILL"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list =="
git ls-files | rg '(^|/)test/presets/node\.test\.ts$|package\.json$|vitest|tsconfig' || true
echo "== file excerpt =="
if [ -f test/presets/node.test.ts ]; then
cat -n test/presets/node.test.ts | sed -n '1,140p'
fi
echo "== package scripts/config =="
if [ -f package.json ]; then
node -e "const p=require('./package.json'); console.log(JSON.stringify({scripts:p.scripts,devDependencies:p.devDependencies,dependencies:p.dependencies}, null, 2))"
fiRepository: nitrojs/nitro
Length of output: 11216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== executable/kill availability =="
command -v node || true
command -v kill || true
node --version || true
echo "== Node child.spawn behavior probe =="
node - <<'JS'
const { spawn } = require('node:child_process');
const script = __filename;
let closed = false;
let exited = false;
const child = spawn(process.execPath, [script]);
child.stdout.on('data', () => {
console.log('READY');
// Stop accepting signals after first READY to keep behavior deterministic.
child.kill = () => {};
});
child.on('close', () => { closed = true; });
child.on('exit', () => { exited = true; });
setTimeout(() => {
// Simulate the first SIGTERM path taking no effect before async exit handling;
// this mirrors a server that simply ignores SIGTERM or does not yet attach the close handler.
child.kill('SIGTERM');
// Force kill only after the process has had time enough to report READY.
setTimeout(() => {
child.kill('SIGKILL');
setTimeout(() => {
console.log(String({ closed, exited, nodeVersion: process.version }));
process.exit(0);
}, 500);
}, 400);
}, 100);
JS
echo "== Vitest lifecycle/error context =="
cat -n vitest.config.ts | sed -n '1,220p'
rg -n "setupFiles|globalTeardown|testTimeout|pool|threads|forks|isolated" test vitest.config.ts package.jsonRepository: nitrojs/nitro
Length of output: 769
🌐 Web query:
Vitest test timeout skip throw skipped test lifecycle hooks after skip
💡 Result:
In Vitest, when a test is marked with.skip (or test.skip) or skipped dynamically via the skip function from the test context, the test body itself is not executed [1][2]. Regarding the lifecycle hooks, the behavior depends on whether the test is skipped statically or dynamically [1][3]: 1. Statically Skipped Tests (.skip): When a test is defined with.skip (e.g., test.skip('name',...)), it is marked as skipped during the test collection phase. Consequently, no lifecycle hooks associated with that specific test (such as beforeEach, afterEach, or test-scoped fixtures) are executed for that test [4][2]. 2. Dynamically Skipped Tests (context.skip): If you call the skip function from within the test context (e.g., inside the test body or a beforeEach hook), Vitest treats this as an instruction to abort the execution of that specific test [3]. When skip is called, it throws an internal "PendingError" to halt execution [3]. Because this occurs after the test lifecycle has already begun, any hooks that ran prior to the call (like beforeEach) will have already executed [4][5]. However, because the test is aborted, subsequent lifecycle steps for that specific test—such as the test body itself, afterEach hooks, and onTestFinished/onTestFailed callbacks—will typically be skipped or behave according to the runner's internal state management for cancelled/skipped tests [4][3][5]. Summary of Behavior: - Static Skipping (.skip): No test-specific lifecycle hooks run. - Dynamic Skipping (context.skip): Lifecycle hooks that precede the skip call will run; hooks scheduled to run after the skip call (including the test body) will not execute [4][5]. Suite-level hooks (beforeAll, afterAll) are generally independent of individual test skips, as they are scoped to the entire suite or file rather than the individual test [4][6]. If a beforeAll hook fails, Vitest marks the remaining tests in that suite as skipped [7].
Citations:
- 1: https://vitest.dev/api/test
- 2: https://vitest.dev/guide/filtering
- 3: How to skip a test if a particular error is thrown? vitest-dev/vitest#4318
- 4: https://vitest.dev/api/hooks.html
- 5: https://github.com/vitest-dev/vitest/blob/206e8cff/docs/guide/lifecycle.md
- 6: https://vitest.dev/guide/learn/setup-teardown
- 7: test state is in "pending" when
beforeAllfailed vitest-dev/vitest#4820
Always terminate the spawned server.
If waitForPort, the close wait, or an assertion throws, Line 85 is skipped and the generated server can remain running. Put the shutdown sequence in a try block and force termination in finally.
🤖 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 `@test/presets/node.test.ts` around lines 61 - 85, The spawned server in the
test flow must always be terminated, including when waitForPort, the close wait,
or an assertion fails. Wrap the startup, waiting, assertions, and close-hook
handling around the child created by execa in a try block, and move the SIGKILL
cleanup into finally while preserving the existing graceful SIGTERM and
close-marker behavior.
1 participant
🔗 Linked issue
Resolves #4502
❓ Type of change
📚 Description
The
node_serverpreset no longer called Nitro's runtimeclosehook on SIGTERM or SIGINT, so cleanup handlers could be skipped in production. Nitro v2 usedsetupGracefulShutdown(listener, nitroApp)and ran the hook after connections drained; v3 delegates shutdown to srvx, which closes the server without calling Nitro hooks.This wraps
server.close()in the node server and cluster entries so the runtime hook runs once, is awaited, and still runs if the underlying close rejects. It runs after the server closes, matching the v2 order. Bun and Deno are unchanged because they did not call the hook in v2 either.The issue reproduction now works as expected: build the
node_serverfixture, start it, send SIGTERM, and the close-hook marker appears before shutdown completes.Verification
pnpm vitest run test/presets/node.test.ts: 66 passed with both rolldown and rollup.The hook shares srvx's normal shutdown budget. The fixture scheduler can keep the process alive after the server closes, so the test kills it after the assertion; that behavior already existed.
NITRO_SHUTDOWN_*remains out of scope.📝 Checklist
Best Regards, Tarik