fix: restore runtime close hook and implement graceful shutdown env vars
#4017
Conversation
Wire NITRO_UNIX_SOCKET, NITRO_SHUTDOWN_DISABLED, and NITRO_SHUTDOWN_TIMEOUT through to srvx serve options in the Node, Bun, and Deno server presets. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
The Handler (advanced) section referenced preset `node` and export `listener`, but `node` is an alias for `node-server` (not the middleware handler). The actual preset name is `node-middleware` and the export is `middleware`. Co-authored-by: Cursor <cursoragent@cursor.com>
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>
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>
Merge wade/implement-env-vars into wade/fix-close-hook to create a comprehensive graceful shutdown PR. Resolves conflict in node docs by keeping NITRO_SHUTDOWN_TIMEOUT and fixes default timeout to 5000ms (srvx default) across all runtime docs. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a centralized shutdown mechanism: parses shutdown env vars, registers SIGTERM/SIGINT handlers that call the Nitro app Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/presets/node/runtime/node-cluster.ts (1)
27-34:⚠️ Potential issue | 🟡 MinorAdd
gracefulShutdownconfiguration tonode-cluster.tsto matchnode-server.ts.The
node-cluster.tspreset is missing thegracefulShutdownconfiguration that readsNITRO_SHUTDOWN_DISABLEDandNITRO_SHUTDOWN_TIMEOUTenvironment variables. Thenode-server.ts,bun.ts, anddeno-server.tspresets all include this configuration, butnode-cluster.tsdoes not. This causes the documented shutdown environment variables to have no effect in cluster mode, creating an inconsistency across Node presets.Add the env var handling from
node-server.ts(lines 18-22) tonode-cluster.tsand pass thegracefulShutdownoption to theserve()call.
🤖 Fix all issues with AI agents
In `@docs/2.deploy/10.runtimes/bun.md`:
- Line 21: The "### Environment Variables" heading is one level too deep
relative to the top-level "# Bun" header; change the "### Environment Variables"
heading text to "## Environment Variables" so the document uses an h2 under "#
Bun" and preserves Markdown heading hierarchy (locate the string "###
Environment Variables" in the file and replace it with "## Environment
Variables").
In `@docs/2.deploy/10.runtimes/deno.md`:
- Line 21: The "Environment Variables" heading currently uses h3 (###) but
should be h2 (##) to follow the document's heading hierarchy under the top-level
"# Deno" heading; update the "Environment Variables" heading to use "##
Environment Variables" so it directly follows "# Deno" and restores correct
heading-level progression.
In `@src/presets/deno/runtime/deno-server.ts`:
- Line 50: The shutdown handler registered by setupShutdownHooks currently calls
useNitroApp().hooks?.callHook("close") without awaiting it, risking truncated
async cleanup; change the handler to an async function and await the
callHook("close") Promise (e.g., make the handler async and use await
useNitroApp().hooks?.callHook("close")), ensuring the returned promise is
awaited before allowing process exit; also consider returning the promise from
setupShutdownHooks so callers can coordinate with srvx or other shutdown
orchestrators.
In `@src/runtime/internal/shutdown.ts`:
- Around line 3-9: Change the sync handler in setupShutdownHooks to an async
function that awaits the application close hook and handles errors: make the
handler async, call await useNitroApp().hooks?.callHook("close") inside a
try/catch, log or report any caught error, and then let the process-level
graceful shutdown logic proceed; keep registering the async handler with
process.on for the signals ("SIGTERM","SIGINT") so the promise is awaited
instead of fire-and-forget.
🧹 Nitpick comments (3)
src/runtime/internal/shutdown.ts (1)
7-8:process.onis a Node.js-specific API used insrc/runtime/.Per coding guidelines, runtime code must not use Node.js-specific APIs unless behind runtime checks. While
processis available in Bun and Deno, it is not available in edge/serverless targets. Since this module lives insrc/runtime/internal/and gets bundled for all deployment targets, consider guarding theprocessreference:for (const sig of ["SIGTERM", "SIGINT"] as const) { - process.on(sig, handler); + if (typeof process !== "undefined" && process.on) { + process.on(sig, handler); + } }This is a low-risk concern since
setupShutdownHooks()is only called from long-running server presets, but it would prevent a reference error if the module is ever inadvertently imported elsewhere. As per coding guidelines: "Code insrc/runtime/must not use Node.js-specific APIs unless behind runtime checks."test/unit/shutdown.test.ts (1)
38-48: Tests don't verify async completion ofcallHook.The current tests only assert that
callHookwas called with"close", but don't verify that the returned promise is awaited. Given the critical issue flagged inshutdown.ts(fire-and-forget promise), consider adding a test that makescallHookreturn a delayed promise and asserts the async work completes before the handler returns:it("awaits the close hook promise", async () => { let resolved = false; callHook.mockImplementation(() => new Promise<void>((r) => { setTimeout(() => { resolved = true; r(); }, 50); })); setupShutdownHooks(); process.emit("SIGTERM", "SIGTERM"); // If the handler awaits, `resolved` should eventually be true await vi.waitFor(() => expect(resolved).toBe(true)); });This would serve as a regression test to ensure the handler properly awaits the hook.
src/presets/bun/runtime/bun.ts (1)
19-25: Consider extracting the duplicatedgracefulShutdowncomputation.This exact block (env var parsing →
false | { gracefulTimeout } | undefined) is duplicated indeno-server.ts,bun.ts, and likelynode-server.ts. A small shared helper (e.g., insrc/runtime/internal/shutdown.tsalongsidesetupShutdownHooks) would reduce the copy-paste surface:Example helper
// in src/runtime/internal/shutdown.ts export function resolveGracefulShutdown(): false | { gracefulTimeout: number } | undefined { if (process.env.NITRO_SHUTDOWN_DISABLED === "true") return false; const timeout = Number.parseInt(process.env.NITRO_SHUTDOWN_TIMEOUT || "", 10); return timeout > 0 ? { gracefulTimeout: timeout / 1000 } : undefined; }
|
|
||
| :read-more{to="https://bun.sh"} | ||
|
|
||
| ### Environment Variables |
There was a problem hiding this comment.
Heading level skips from h1 to h3.
There's no h2 between # Bun (line 5) and ### Environment Variables (line 21). This should be ## to comply with Markdown heading hierarchy.
📝 Suggested fix
-### Environment Variables
+## Environment Variables🤖 Prompt for AI Agents
In `@docs/2.deploy/10.runtimes/bun.md` at line 21, The "### Environment Variables"
heading is one level too deep relative to the top-level "# Bun" header; change
the "### Environment Variables" heading text to "## Environment Variables" so
the document uses an h2 under "# Bun" and preserves Markdown heading hierarchy
(locate the string "### Environment Variables" in the file and replace it with
"## Environment Variables").
| deno run --unstable --allow-net --allow-read --allow-env .output/server/index.ts | ||
| ``` | ||
|
|
||
| ### Environment Variables |
There was a problem hiding this comment.
Same heading-level skip as bun.md — should be ## not ###.
The file uses # Deno (h1) at line 5 and jumps to ### Environment Variables (h3) at line 21, skipping h2.
📝 Suggested fix
-### Environment Variables
+## Environment Variables📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Environment Variables | |
| ## Environment Variables |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 21-21: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
In `@docs/2.deploy/10.runtimes/deno.md` at line 21, The "Environment Variables"
heading currently uses h3 (###) but should be h2 (##) to follow the document's
heading hierarchy under the top-level "# Deno" heading; update the "Environment
Variables" heading to use "## Environment Variables" so it directly follows "#
Deno" and restores correct heading-level progression.
Co-authored-by: Cursor <cursoragent@cursor.com>
Await callHook("close") so async cleanup (telemetry flush, db close,
etc.) completes before the process exits. Catch and log errors so a
failing hook doesn't silently swallow the exception.
Co-authored-by: Cursor <cursoragent@cursor.com>
Lift the duplicated NITRO_SHUTDOWN_DISABLED / NITRO_SHUTDOWN_TIMEOUT env var parsing into a shared helper in the shutdown module. All three server presets (node, bun, deno) now import it instead of inlining the same logic. Co-authored-by: Cursor <cursoragent@cursor.com>
- Add srvx ServerOptions type to resolveGracefulShutdownConfig return - Extract _shutdownHandler as a named module-level async function - Check NITRO_SHUTDOWN_DISABLED as truthy instead of strict "true" - Reformat long import/expression lines across preset runtime entries Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/runtime/internal/shutdown.ts`:
- Around line 4-7: The current resolveGracefulShutdownConfig function treats any
non-empty NITRO_SHUTDOWN_DISABLED value as "disabled", so change the check to
parse the env var explicitly (normalize to lowercase/trim) and only treat known
truthy values (e.g. "1", "true", "yes") as disabling; use that parsed result to
return false for graceful shutdown, and otherwise fall through to the normal
behavior. Target the resolveGracefulShutdownConfig
function/ServerOptions["gracefulShutdown"] logic and add an explicit
whitelist/blacklist of accepted boolean strings when reading
process.env.NITRO_SHUTDOWN_DISABLED.
- Around line 27-30: The setupShutdownHooks function currently calls
process.on("SIGTERM"/"SIGINT") directly; wrap these calls in a runtime guard
that checks for the existence of a process object and that process.on is
available before registering _shutdownHandler (e.g., guard using typeof process
!== 'undefined' && process.on). Update the function setupShutdownHooks to
perform that guard around both signal registrations so Node-specific API usage
is only executed when the runtime exposes process.on.
Guard process.on calls in setupShutdownHooks with a typeof check to prevent reference errors if the module is ever imported in edge/serverless targets where process is unavailable. Add regression test verifying the handler awaits a delayed close hook promise to completion. Co-authored-by: Cursor <cursoragent@cursor.com>
Match the documented behavior ("set to 'true'") instead of treating any
truthy string as disabled. Add tests for "false" and empty string values.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/runtime/internal/shutdown.ts`:
- Around line 19-31: _shutdownHandler can be invoked twice by multiple signals;
add an idempotency guard to prevent reentrant calls: introduce a module-level
boolean (e.g., isShuttingDown) checked at the start of _shutdownHandler and set
true before awaiting useNitroApp().hooks?.callHook("close"), so subsequent
SIGTERM/SIGINT handlers return early; also remove or ignore the signal listeners
in setupShutdownHooks (or ensure process.on still attaches but relies on the
boolean) so hooks.callHook("close") only runs once and plugins aren't cleaned up
twice.
src/runtime/internal/shutdown.ts
Outdated
| async function _shutdownHandler() { | ||
| try { | ||
| await useNitroApp().hooks?.callHook("close"); | ||
| } catch (error) { | ||
| console.error("[nitro] Error running close hook:", error); | ||
| } | ||
| } | ||
|
|
||
| export function setupShutdownHooks() { | ||
| if (typeof process !== "undefined" && process.on) { | ||
| process.on("SIGTERM", _shutdownHandler); | ||
| process.on("SIGINT", _shutdownHandler); | ||
| } |
There was a problem hiding this comment.
Double signal delivery can invoke the close hook twice.
In production, it's common for orchestrators (Docker, Kubernetes, systemd) to send SIGTERM and then follow up with SIGINT if the process hasn't exited. Both signals would trigger _shutdownHandler, calling the close hook a second time while the first may still be in-flight. This can cause errors in plugin cleanup code (e.g., closing an already-closed database connection).
Consider guarding with a flag:
Proposed fix
+let _shutting = false;
+
async function _shutdownHandler() {
+ if (_shutting) {
+ return;
+ }
+ _shutting = true;
try {
await useNitroApp().hooks?.callHook("close");
} catch (error) {
console.error("[nitro] Error running close hook:", error);
}
}🧰 Tools
🪛 Biome (2.3.13)
[error] 21-21: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In `@src/runtime/internal/shutdown.ts` around lines 19 - 31, _shutdownHandler can
be invoked twice by multiple signals; add an idempotency guard to prevent
reentrant calls: introduce a module-level boolean (e.g., isShuttingDown) checked
at the start of _shutdownHandler and set true before awaiting
useNitroApp().hooks?.callHook("close"), so subsequent SIGTERM/SIGINT handlers
return early; also remove or ignore the signal listeners in setupShutdownHooks
(or ensure process.on still attaches but relies on the boolean) so
hooks.callHook("close") only runs once and plugins aren't cleaned up twice.
There was a problem hiding this comment.
I declined to change this to follow the pattern from the pre-existing error handling hooks.
This is likely a good safety feature, though double-closes are in most cases no-op and/or can be handled in the application, depending on how abstracted/magic the hooks system should be–preferably less, imo, since hooks are by nature kind of a break through the abstraction wall.
There was a problem hiding this comment.
@wadefletch, thank you for the explanation. That makes sense—keeping the hooks layer transparent and consistent with the existing error-handling patterns is a reasonable architectural choice. Application-level code can guard against double-close if needed, and avoiding extra magic in the hooks system preserves the "break through the abstraction wall" clarity you described. I appreciate the insight into the design philosophy.
✏️ Learnings added
Learnt from: wadefletch
Repo: nitrojs/nitro PR: 4017
File: src/runtime/internal/shutdown.ts:0-0
Timestamp: 2026-02-10T16:16:53.352Z
Learning: In Nitro's shutdown hooks (src/runtime/internal/shutdown.ts), double-signal delivery (e.g., SIGTERM followed by SIGINT) is intentionally not guarded with idempotency flags. The design follows pre-existing error handling hook patterns and prefers less abstraction/magic in the hooks system, allowing application-level code to handle double-close scenarios if needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Prevent the close hook from firing multiple times when both SIGTERM and SIGINT arrive (e.g. Ctrl+C during a SIGTERM shutdown). A module-level boolean short-circuits duplicate invocations. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop the async handler, idempotency guard, runtime check, and error logging to match the pattern used by the sibling trapUnhandledErrors utility. Keep it minimal: process.on + callHook. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
| return false; | ||
| } | ||
|
|
||
| const timeoutMs = Number.parseInt(process.env.NITRO_SHUTDOWN_TIMEOUT ?? "", 10); |
There was a problem hiding this comment.
I think it is best that we don't introduce new NITRO_ env vars where srvx already supports SERVER_SHUTDOWN_TIMEOUT
| const cert = process.env.NITRO_SSL_CERT; | ||
| const key = process.env.NITRO_SSL_KEY; | ||
| // const socketPath = process.env.NITRO_UNIX_SOCKET; // TODO | ||
| const socketPath = process.env.NITRO_UNIX_SOCKET; |
There was a problem hiding this comment.
We could introduce this also in srvx directly via an env without NITRO_.
2 participants
🔗 Linked issue
nitro.hooks.callHook("close")on shutdown #4015NITRO_SHUTDOWNenv variable #2376process.exit(0)in graceful shutdown plugin prevents application-level async cleanup h3js/srvx#178 / fix: avoidprocess.exitafter graceful server close h3js/srvx#179 (process.exit(0)after graceful shutdown prevents application-level async cleanup from completing)❓ 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.This PR restores the application-level close hook AND wires through the documented-but-unimplemented graceful shutdown env vars to srvx.
New
src/runtime/internal/shutdown.tsmodulesetupShutdownHooks()registers SIGTERM/SIGINT handlers thatawaitthe Nitroclosehook, withtry/catcherror logging. Follows the same pattern as the existingtrapUnhandledErrors()utility.resolveGracefulShutdownConfig()readsNITRO_SHUTDOWN_DISABLEDandNITRO_SHUTDOWN_TIMEOUTenv vars and returns the srvx-compatiblegracefulShutdownconfig. Typed withServerOptions["gracefulShutdown"]from srvx.node-server,node-cluster,bun,deno-server), eliminating duplicated env var parsing.Implement env vars across runtimes
Wires documented-but-unimplemented env vars through to srvx
serve()options in the Node, Bun, and Deno server presets:NITRO_UNIX_SOCKET: Passes socket path to srvx vianode: { path }(Node.js) andbun: { unix }(Bun). Not available for Deno asDeno.servedoes not support UNIX sockets.NITRO_SHUTDOWN_DISABLED: Maps togracefulShutdown: falsewhen set (any truthy value).NITRO_SHUTDOWN_TIMEOUT: Maps togracefulShutdown: { gracefulTimeout }(converted from ms to seconds for srvx).Remove env vars that cannot be configured through srvx
Two env vars were previously documented but cannot be implemented through srvx's current API, so they are removed from the docs rather than left as misleading:
NITRO_SHUTDOWN_SIGNALS-- srvx hardcodes SIGINT/SIGTERMNITRO_SHUTDOWN_FORCE-- srvx always callsprocess.exit(0)Documentation updates
"close"to the available hooks list in plugin docs, expanded graceful shutdown section with example, clarified that the close hook is only available on long-running server presets"response"hook signature in plugin docs ((response, event)not(event, { body }))Alignment with srvx
Verified against srvx v0.11.2 (which nitro is pinned to via
^0.11.2). The graceful shutdown plugin was refactored in h3js/srvx@54e7074:gracefulTimeoutis the only config option the implementation readsprocess.exit(0)is always called after closeImportant: srvx currently calls
process.exit(0)immediately after graceful server close (h3js/srvx#178), which kills the process before application-level async cleanup (like theclosehook handlers registered bysetupShutdownHooks()) can complete. h3js/srvx#179 fixes this by replacingprocess.exit(0)withreturnon the happy path so the event loop drains naturally. Until that lands, theclosehook will fire but async handlers may be interrupted before finishing.📝 Checklist