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
Problem: With the
node_serverpreset, the runtimeclosehook never fires on SIGTERM/SIGINT in production, so plugin cleanup handlers (flush, checkpoint, resource disposal) are silently skipped. This is a regression from v2, where the node-server entry calledsetupGracefulShutdown(listener, nitroApp)whoseonShutdownrannitroApp.hooks.callHook("close"). In v3 the entry only calls srvxserve(), and srvx'sgracefulShutdownPlugincloses the server without touching Nitro hooks.Fix: In the node runtime entries (
node-server.ts,node-cluster.ts), wrapserver.close()so that runtimeclosehooks run (once, awaited) whenever the server is closed — including via srvx's graceful shutdown onSIGINT/SIGTERM, which callsserver.close()on the instance. Hooks run after the underlying close completes (matching v2, wherehttp-graceful-shutdownranonShutdownafter connections drained), and also run if the underlying close rejects (try/finally).node_clusterhad the same regression (v2 cluster workers ran the node-server entry). Bun/Deno presets are intentionally untouched: they never calledclosehooks in v2 either, so there is no regression to restore there.callHookreturnsPromise | void(void when all hooks are synchronous), hence?.catch, matching the existing runtime pattern.Reproduction (from the issue): fixture plugin registers
app.hooks.hook("close", ...), build withnode_serverpreset, run.output/server/index.mjs, send SIGTERM. Before: hook never runs (Server closed successfully.with no hook output). After: hook runs before shutdown completes.Verification:
nitro:preset:node-server > calls the close hook on shutdownbuilds the fixture, spawns the built entry, waits for the port, sends SIGTERM and asserts the hook ran and nounhandledRejectionoccurred. It fails without the fix and passes with it (verified both ways).pnpm vitest run test/presets/node.test.ts— 66 passed with bothrolldownandrollupbuilders (NITRO_BUILDER=rollup). This also adds the firstnode_serverpreset coverage.node .output/server/index.mjs+ SIGTERM prints the close-hook marker followed byServer closed successfully.pnpm lint,pnpm fmt,pnpm typecheck— clean.try/finally).Notes / limitations:
SERVER_SHUTDOWN_TIMEOUT); a secondSIGINT/SIGTERMstill force-closes connections while slow hooks run (guarded to run once).NITRO_SHUTDOWN_*env vars remain unwired (srvx owns shutdown now); restoring those is out of scope, thoughdocs/2.deploy/10.runtimes/1.node.mdstill documents them.test/presets/nitro-dev.test.tsfails identically on cleanmainin my environment (Invalid URLfrom the dev server harness) — pre-existing and unrelated to this change.node_clusterhas no dedicated test yet.📝 Checklist
closehook semantics)