fix: call runtime close hook on shutdown signals
#4016
Conversation
Bridge SIGTERM/SIGINT to `nitroApp.hooks.callHook("close")` so plugins
can run async cleanup (flush telemetry, drain connections, stop queues)
when the server shuts down.
The close hook stopped firing after the srvx migration in nitrojs#3705 removed
the old `setupGracefulShutdown` machinery. srvx handles HTTP-level
shutdown (connection draining) but never calls Nitro's application-level
close hook.
Adds `setupShutdownHooks()` utility following the same pattern as
`trapUnhandledErrors()` and wires it into node-server, node-cluster,
bun, and deno-server runtime entries.
Resolves nitrojs#4015
Resolves nitrojs#2735
Resolves nitrojs#2566
Co-authored-by: Cursor <cursoragent@cursor.com>
|
@wadefletch is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds runtime shutdown wiring: a new setupShutdownHooks utility registers SIGTERM/SIGINT handlers to call Nitro's "close" hook; this utility is invoked in Bun, Deno, Node (server & cluster) presets. Documentation and tests for shutdown behavior were added/updated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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: 3
🤖 Fix all issues with AI agents
In `@docs/2.deploy/10.runtimes/1.node.md`:
- Line 36: The docs mention NITRO_SHUTDOWN_DISABLED but the runtime always
registers shutdown hooks; update setupShutdownHooks in
src/runtime/internal/shutdown.ts to read process.env.NITRO_SHUTDOWN_DISABLED
(string) and, when it equals 'true', skip registering signal handlers and return
early (or log that graceful shutdown is disabled), otherwise proceed to register
the handlers as before; ensure the check uses the exact env var name and is
evaluated before any calls to process.on or similar so the documented behavior
matches the implementation.
In `@src/runtime/internal/shutdown.ts`:
- Around line 3-9: In setupShutdownHooks replace the fire-and-forget handler
with an async guarded handler that (1) tracks a local boolean (e.g.,
"shuttingDown") to prevent re-entrance, (2) awaits
useNitroApp().hooks.callHook("close") inside try/catch, (3) on success calls
process.exit(0) and on error logs the error and calls process.exit(1), and (4)
registers that async handler for "SIGTERM" and "SIGINT" (and optionally sets a
hard timeout to force exit) so the process does not hang; update the handler
identifier and the call to useNitroApp().hooks.callHook("close") accordingly and
ensure listeners are removed or ignored after the first invocation.
In `@test/unit/shutdown.test.ts`:
- Around line 38-48: The shutdown handler currently calls
useNitroApp().hooks?.callHook("close") without awaiting the returned Promise;
change the handler in src/runtime/internal/shutdown.ts (the function registered
by setupShutdownHooks) to be async and await
useNitroApp().hooks?.callHook("close") so async cleanup finishes before exit.
Update tests in test/unit/shutdown.test.ts to mock callHook as an async mock
(e.g., jest.fn().mockResolvedValue(undefined)), make the tests async, emit the
signal via process.emit, then await a microtask flush (await Promise.resolve()
or await new Promise(setImmediate)) before asserting
expect(callHook).toHaveBeenCalledWith("close") so the promise resolution is
observed.
🧹 Nitpick comments (2)
test/unit/shutdown.test.ts (2)
38-48: No test for repeated signal delivery — theclosehook could run multiple times.In practice, a process can receive SIGTERM followed by SIGINT (e.g., Kubernetes sends SIGTERM, then the user hits Ctrl+C). The current implementation will call
callHook("close")for each signal. Depending on what plugins do in their close hooks, running them twice could cause errors (double-free, duplicate flushes, etc.).Consider adding a test that asserts
callHookis called only once even when both signals fire, and guard the implementation accordingly (e.g., with alet closing = falseflag).
14-15: Minor: preferNodeJS.SignalsListener[]overFunction[]for saved listeners.Lines 14–15 declare
Function[]but the cast on lines 26–27 usesNodeJS.SignalsListener. Using the correct type from the start avoids the cast:- let savedSIGTERM: Function[]; - let savedSIGINT: Function[]; + let savedSIGTERM: NodeJS.SignalsListener[]; + let savedSIGINT: NodeJS.SignalsListener[];Then lines 26-27 won't need the
ascast.
| - `NITRO_SHUTDOWN_SIGNALS` - Allows you to specify which signals should be handled. Each signal should be separated with a space. Defaults to `'SIGINT SIGTERM'`. | ||
| - `NITRO_SHUTDOWN_TIMEOUT` - Sets the amount of time (in milliseconds) before a forced shutdown occurs. Defaults to `'30000'` milliseconds. | ||
| - `NITRO_SHUTDOWN_FORCE` - When set to true, it triggers `process.exit()` at the end of the shutdown process. If it's set to `'false'`, the process will simply let the event loop clear. Defaults to `'true'`. | ||
| - `NITRO_SHUTDOWN_DISABLED` - Disables the graceful shutdown feature when set to `'true'`. Defaults to `'false'`. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if NITRO_SHUTDOWN_DISABLED is referenced anywhere in the runtime/preset code
rg -n "NITRO_SHUTDOWN_DISABLED" --type=ts -g '!docs/**'Repository: nitrojs/nitro
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Find the shutdown.ts file
fd "shutdown\.ts" -type f
# Also check the documentation file
cat -n docs/2.deploy/10.runtimes/1.node.md | sed -n '30,45p'Repository: nitrojs/nitro
Length of output: 1193
🏁 Script executed:
#!/bin/bash
# Find the shutdown file properly
find . -name "*shutdown*" -type f | grep -v node_modules | grep -v dist | head -20
# Also search for setupShutdownHooks mentioned in the review
rg "setupShutdownHooks" -g '!docs/**' -g '!node_modules/**'Repository: nitrojs/nitro
Length of output: 1161
🏁 Script executed:
#!/bin/bash
# Read the shutdown.ts file to see if it checks NITRO_SHUTDOWN_DISABLED
cat -n ./src/runtime/internal/shutdown.tsRepository: nitrojs/nitro
Length of output: 372
Remove NITRO_SHUTDOWN_DISABLED from documentation or implement the feature in code.
The environment variable NITRO_SHUTDOWN_DISABLED is documented on line 36 but does not exist in the implementation. The setupShutdownHooks() function in src/runtime/internal/shutdown.ts unconditionally registers signal handlers with no check for this env var. Users will be misled by the documentation. Either implement the env var check in setupShutdownHooks() to conditionally skip shutdown hook registration, or remove this env var from the docs.
🤖 Prompt for AI Agents
In `@docs/2.deploy/10.runtimes/1.node.md` at line 36, The docs mention
NITRO_SHUTDOWN_DISABLED but the runtime always registers shutdown hooks; update
setupShutdownHooks in src/runtime/internal/shutdown.ts to read
process.env.NITRO_SHUTDOWN_DISABLED (string) and, when it equals 'true', skip
registering signal handlers and return early (or log that graceful shutdown is
disabled), otherwise proceed to register the handlers as before; ensure the
check uses the exact env var name and is evaluated before any calls to
process.on or similar so the documented behavior matches the implementation.
| export function setupShutdownHooks() { | ||
| const handler = () => { | ||
| useNitroApp().hooks?.callHook("close"); | ||
| }; | ||
| for (const sig of ["SIGTERM", "SIGINT"] as const) { | ||
| process.on(sig, handler); | ||
| } |
There was a problem hiding this comment.
Critical: handler does not await the close hook and never exits the process.
Two interrelated problems:
callHook("close")returns aPromise, but the handler neitherawaits it nor chains.then(). Async cleanup (flushing telemetry, draining DB pools) will not complete — this directly defeats the objective of issue#2735.- Registering a listener on
SIGTERM/SIGINTsuppresses the default OS termination behavior. Since the handler returns immediately (fire-and-forget promise) and never callsprocess.exit(), the process will hang indefinitely after receiving the signal.
Additionally, there's no guard against double invocation — a rapid Ctrl+C twice, or SIGTERM followed by SIGINT, will call the close hook multiple times, which may cause errors in plugins that tear down resources once.
Proposed fix
export function setupShutdownHooks() {
+ let shutdownPromise: Promise<void> | undefined;
const handler = () => {
- useNitroApp().hooks?.callHook("close");
+ if (shutdownPromise) {
+ return;
+ }
+ shutdownPromise = (useNitroApp().hooks?.callHook("close") ?? Promise.resolve())
+ .finally(() => {
+ process.exit(0);
+ });
};
for (const sig of ["SIGTERM", "SIGINT"] as const) {
process.on(sig, handler);
}
}🤖 Prompt for AI Agents
In `@src/runtime/internal/shutdown.ts` around lines 3 - 9, In setupShutdownHooks
replace the fire-and-forget handler with an async guarded handler that (1)
tracks a local boolean (e.g., "shuttingDown") to prevent re-entrance, (2) awaits
useNitroApp().hooks.callHook("close") inside try/catch, (3) on success calls
process.exit(0) and on error logs the error and calls process.exit(1), and (4)
registers that async handler for "SIGTERM" and "SIGINT" (and optionally sets a
hard timeout to force exit) so the process does not hang; update the handler
identifier and the call to useNitroApp().hooks.callHook("close") accordingly and
ensure listeners are removed or ignored after the first invocation.
| it("calls close hook on SIGTERM", () => { | ||
| setupShutdownHooks(); | ||
| process.emit("SIGTERM", "SIGTERM"); | ||
| expect(callHook).toHaveBeenCalledWith("close"); | ||
| }); | ||
|
|
||
| it("calls close hook on SIGINT", () => { | ||
| setupShutdownHooks(); | ||
| process.emit("SIGINT", "SIGINT"); | ||
| expect(callHook).toHaveBeenCalledWith("close"); | ||
| }); |
There was a problem hiding this comment.
The close hook's promise is neither awaited in the implementation nor verified in the test.
Looking at the implementation in src/runtime/internal/shutdown.ts:
const handler = () => {
useNitroApp().hooks?.callHook("close");
};callHook returns a Promise, but the handler doesn't await it. This means async cleanup (flushing telemetry, draining DB pools, etc.) can be cut short when the process exits — which is exactly the bug described in issue #2735.
The test should use an async mock to surface this:
Suggested test improvement (after fixing the implementation to await)
it("calls close hook on SIGTERM", async () => {
+ callHook.mockResolvedValueOnce(undefined);
setupShutdownHooks();
- process.emit("SIGTERM", "SIGTERM");
+ await process.emit("SIGTERM", "SIGTERM");
expect(callHook).toHaveBeenCalledWith("close");
});And in the implementation:
- const handler = () => {
- useNitroApp().hooks?.callHook("close");
+ const handler = async () => {
+ await useNitroApp().hooks?.callHook("close");
};🤖 Prompt for AI Agents
In `@test/unit/shutdown.test.ts` around lines 38 - 48, The shutdown handler
currently calls useNitroApp().hooks?.callHook("close") without awaiting the
returned Promise; change the handler in src/runtime/internal/shutdown.ts (the
function registered by setupShutdownHooks) to be async and await
useNitroApp().hooks?.callHook("close") so async cleanup finishes before exit.
Update tests in test/unit/shutdown.test.ts to mock callHook as an async mock
(e.g., jest.fn().mockResolvedValue(undefined)), make the tests async, emit the
signal via process.emit, then await a microtask flush (await Promise.resolve()
or await new Promise(setImmediate)) before asserting
expect(callHook).toHaveBeenCalledWith("close") so the promise resolution is
observed.
commit: |
Note that the close hook only fires on long-running server presets (node, bun, deno) and not in serverless/edge environments. Guide plugin authors toward the response hook or waitUntil for per-request cleanup that works across all presets. Co-authored-by: Cursor <cursoragent@cursor.com>
wadefletch
force-pushed
the
wade/fix-close-hook
branch
from
5630412 to
ea4235a
Compare
1 participant
🔗 Linked issue
nitro.hooks.callHook("close")on shutdown #4015❓ Type of change
📚 Description
The runtime
closehook stopped firing after the srvx migration in #3705 removed the oldsetupGracefulShutdownmachinery. srvx handles HTTP-level shutdown (connection draining) but never calls Nitro's application-levelclosehook, making it impossible for plugins to run async cleanup on shutdown (flush telemetry, drain database connections, stop job queues, etc.).Root cause:
src/runtime/internal/shutdown.tsand itssetupGracefulShutdown()function (which listened for SIGTERM/SIGINT and calleduseNitroHooks().callHook("close")) were deleted in #3705 when the presets were rewritten to usesrvx. The HTTP shutdown was delegated to srvx'sgracefulShutdownPlugin, but the bridge from OS signals to Nitro's hook system was never recreated.Fix: Adds a
setupShutdownHooks()utility that registers SIGTERM/SIGINT handlers callingnitroApp.hooks.callHook("close"). Follows the same pattern as the existingtrapUnhandledErrors()utility -- a small function imported and called from each runtime entry.Changes:
src/runtime/internal/shutdown.tswithsetupShutdownHooks()node-server,node-cluster,bun, anddeno-serverruntime entries"close"to the available hooks list, expanded graceful shutdown section with example, clarified that the close hook is only available on long-running server presetsNITRO_SHUTDOWN_SIGNALS,NITRO_SHUTDOWN_TIMEOUT, andNITRO_SHUTDOWN_FORCEenv vars from Node.js page (these are no longer configurable through srvx)📝 Checklist