feat(meteor): add Meteor.onShutdown() lifecycle hook#14434
feat(meteor): add Meteor.onShutdown() lifecycle hook#14434dupontbertrand wants to merge 5 commits into
Conversation
✅ Deploy Preview for v3-meteor-api-docs canceled.
|
✅ Deploy Preview for v3-migration-docs canceled.
|
|
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 server shutdown infrastructure and a public registration API ChangesServer Shutdown
🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tools/static-assets/server/boot.js`:
- Around line 473-477: The safety timeout created by setTimeout (the variable
timer) must not be unreferenced because timer.unref() can allow the event loop
to empty and prevent the timeout from firing; remove the call to timer.unref()
so the timeout reliably keeps the process alive until either clearTimeout(timer)
is called or it fires and calls process.exit(exitCode), leaving the existing
clearTimeout(timer) and the timeoutMs/exitCode behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a069aa43-2e0e-4e97-b82d-7ff730a7a183
📒 Files selected for processing (3)
docs/source/api/core.mdpackages/meteor/startup_server.jstools/static-assets/server/boot.js
| // Setting this to null tells Meteor.shutdown that shutdown has begun. | ||
| __meteor_bootstrap__.shutdownHooks = null; | ||
|
|
||
| var timeoutMs = parseInt(process.env.METEOR_SHUTDOWN_TIMEOUT_MS, 10) || 10000; |
There was a problem hiding this comment.
METEOR_SHUTDOWN_TIMEOUT_MS=0 cannot disable the timeout, is it intentional?
parseInt('0', 10) || 10000 evaluates to 10000, so 0 (and any invalid value) silently falls back to the default (10000). If "no timeout" isn't meant to be supported, worth documenting that; if you want to allow it, prefer an explicit check:
There was a problem hiding this comment.
Nice catch. I think the user should be able to disable it completely, but I find using 0 to disable it a bit ambiguous.
To me, 0 could potentially mean two very different things: either an “infinite timeout” or an “immediate exit”.
What would you recommend?
There was a problem hiding this comment.
interesting, IMO:
0 -> infinite timeout
1 -> immediate exit
I mean, I havent a strong argument about this DX, you can follow what you feel more confortable
There was a problem hiding this comment.
Went with your suggestion: 0 = no cap (wait indefinitely), a small value like 1 exits almost immediately, and invalid/negative values now fall back to the 10000ms default with a warning instead of being silently coerced. Documented in core.md.
| var shutdownInProgress = false; | ||
|
|
||
| var callShutdownHooks = Profile("Call Meteor.shutdown hooks", async function (signal) { | ||
| if (shutdownInProgress) return; |
There was a problem hiding this comment.
i'm not sure but I guess we loses the "double Ctrl-C to force exit" escape hatch
Since the SIGINT/SIGTERM listeners suppress Node's default termination behavior, a second signal also lands here and is discarded by the return. If a hook hangs, the operator is stuck until METEOR_SHUTDOWN_TIMEOUT_MS (10s default) with no way out
There was a problem hiding this comment.
Good catch — fixed. A second SIGTERM/SIGINT received while shutdown is already in flight now forces an immediate process.exit instead of being swallowed, so the double-Ctrl-C "force quit" escape hatch is back when a hook hangs.
| */ | ||
| Meteor.shutdown = function shutdown(callback) { | ||
| callback = Meteor.wrapFn(callback); | ||
| var bootstrap = global.__meteor_bootstrap__; |
There was a problem hiding this comment.
could we use let/const instead var?
There was a problem hiding this comment.
Done — const/let throughout the shutdown internals (both startup_server.js and boot.js).
Symmetric to Meteor.startup(), Meteor.shutdown(fn) registers a callback fired on SIGTERM / SIGINT before process exit. Useful for closing DB connections, flushing queues, releasing locks, etc. Hooks run in LIFO order sequentially with await. Per-hook errors are logged but do not abort subsequent hooks (best-effort cleanup). Total runtime is capped by METEOR_SHUTDOWN_TIMEOUT_MS (default 10000ms) to avoid stalling supervisor escalation (Galaxy, K8s, systemd) to SIGKILL. Exit codes follow POSIX: SIGINT -> 130, SIGTERM -> 143. Forum thread: https://forums.meteor.com/t/is-there-something-like-meteor-shutdown/64602
Adds the JSDoc @summary block consumed by the apibox tag plus an entry in docs/source/api/core.md, placed right after Meteor.startup since the two are conceptually paired. The prose covers: LIFO ordering with sequential await, best-effort error handling, hard timeout via METEOR_SHUTDOWN_TIMEOUT_MS, POSIX exit codes, server-only locus.
An unref()-ed timer does not keep the event loop alive. If a shutdown hook hangs on a promise with no other pending I/O (e.g. after earlier hooks have closed the server's sockets and Mongo connection), the loop empties and the process exits 0 before the timeout fires, defeating the documented METEOR_SHUTDOWN_TIMEOUT_MS hard cap. clearTimeout() already prevents the timer from delaying the fast path, so unref() only weakened the guarantee.
…meout Addresses italojs's review (CHANGES_REQUESTED, 2026-06-02): - Rename the public API Meteor.shutdown -> Meteor.onShutdown. The "on" prefix reads as a hook registration rather than an imperative "shut the server down now". - A second SIGTERM/SIGINT during an in-flight shutdown now forces an immediate process.exit instead of being swallowed, restoring the conventional double-Ctrl-C "force quit" escape hatch when a hook hangs. - METEOR_SHUTDOWN_TIMEOUT_MS gains explicit semantics: 0 disables the cap (wait for hooks indefinitely), a small value (e.g. 1) exits almost immediately, and invalid/negative values fall back to the 10000ms default with a warning instead of being silently coerced. - Use const/let instead of var in the shutdown internals. Signal coverage stays SIGINT/SIGTERM only; SIGHUP and 'exit' remain out of scope for a follow-up.
7d808fa to
13c7fbd
Compare
|
Heads-up: I also resolved open question #1 by renaming the API |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/meteor/startup_server.js (1)
33-49: ⚡ Quick winMissing stack capture for profiling parity with
Meteor.startup.
Meteor.startupcaptures a stack trace whenMETEOR_PROFILEis enabled and attaches it ascallback.stack(lines 3-16). The downstreamcallShutdownHooksin boot.js useshook.stack || "(unknown)"for profile labels. Without the same capture here, all shutdown hooks will appear as "(unknown)" in profiling output.♻️ Suggested fix to add stack capture
Meteor.onShutdown = function onShutdown(callback) { callback = Meteor.wrapFn(callback); + if (process.env.METEOR_PROFILE) { + var error = new Error("Meteor.onShutdown"); + Error.captureStackTrace(error, onShutdown); + callback.stack = error.stack + .split(/\n\s*/) + .slice(0, 2) + .join(" ") + .replace(/^Error: /, ""); + } const bootstrap = global.__meteor_bootstrap__; if (bootstrap && bootstrap.shutdownHooks) {🤖 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 `@packages/meteor/startup_server.js` around lines 33 - 49, Add the same profiling stack capture that Meteor.startup does to Meteor.onShutdown: when METEOR_PROFILE is set, capture a new Error().stack (or equivalent) and assign it to callback.stack before wrapping/queueing the callback so downstream callShutdownHooks can use hook.stack instead of "(unknown)"; update Meteor.onShutdown (the function handling bootstrap.shutdownHooks and the late microtask branch) to set callback.stack accordingly.
🤖 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.
Nitpick comments:
In `@packages/meteor/startup_server.js`:
- Around line 33-49: Add the same profiling stack capture that Meteor.startup
does to Meteor.onShutdown: when METEOR_PROFILE is set, capture a new
Error().stack (or equivalent) and assign it to callback.stack before
wrapping/queueing the callback so downstream callShutdownHooks can use
hook.stack instead of "(unknown)"; update Meteor.onShutdown (the function
handling bootstrap.shutdownHooks and the late microtask branch) to set
callback.stack accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de55c243-c584-4273-9720-5a5f9bb8a98a
📒 Files selected for processing (3)
docs/source/api/core.mdpackages/meteor/startup_server.jstools/static-assets/server/boot.js
✅ Files skipped from review due to trivial changes (1)
- docs/source/api/core.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/static-assets/server/boot.js
…OR_PROFILE Mirror Meteor.startup's profiling stack capture so shutdown hooks show their registration call-site in the profiler instead of "(unknown)". callShutdownHooks already reads hook.stack; this populates it.
Summary
This PR adds a server-side
Meteor.onShutdown(fn)lifecycle hook, intended as the shutdown counterpart toMeteor.startup(fn).Registered hooks are called on
SIGTERMandSIGINTbefore the process exits, allowing applications to flush logs, drain queues, release locks, close sockets, or finish pending async cleanup before the supervisor escalates toSIGKILL.Why
Meteor exposes
Meteor.startup(fn)for the boot side of the lifecycle, but there's no symmetric API for shutdown. Today, applications that need to clean up on termination attach rawprocess.on('SIGTERM', …)handlers themselves, and the details that matter — per-hook timeout, error containment, ordering, single-point-of-exit — have to be reimplemented in each app.This PR codifies the missing half of the lifecycle so applications can rely on a consistent, framework-level shutdown hook.
Forum context: https://forums.meteor.com/t/is-there-something-like-meteor-shutdown/64602
Use cases
Patterns this enables:
pino,winston, Sentry, OpenTelemetry exporters all have async flush methods that need to complete before exit.What changed
tools/static-assets/server/boot.js—shutdownHooks: []bucket,callShutdownHooks(signal)runner,SIGTERM/SIGINTlisteners.packages/meteor/startup_server.js— publicMeteor.onShutdown(fn)API.docs/source/api/core.md— apibox entry + prose, placed right afterMeteor.startup.Usage
Hooks receive the triggering signal name (
'SIGTERM'or'SIGINT') as their argument.Design choices
Meteor.onShutdownonprefix reads as a hook registration, not an imperative "shut the server down now"for…await)METEOR_SHUTDOWN_TIMEOUT_MS(default 10000ms)SIGKILL— exit well beforeMETEOR_SHUTDOWN_TIMEOUT_MS=01exits almost immediatelyprocess.exitMeteor.startup's "execute immediately when bootstrap is done" pathExit codes follow POSIX:
SIGINT→ 130,SIGTERM→ 143.Review feedback resolved
Following @italojs's review (the four open questions the draft was waiting on are now settled):
Meteor.shutdown→Meteor.onShutdown.METEOR_SHUTDOWN_TIMEOUT_MS—0= no cap,1≈ immediate, invalid/negative → default10000with a warning (no more silent coercion).SIGTERM/SIGINTduring shutdown now forces an immediate exit instead of being swallowed.const/letthroughout the new code.Signal coverage stays
SIGINT/SIGTERMonly;SIGHUPand Node's'exit'remain out of scope for a follow-up.Known limitations / out of scope
This PR does not refactor existing shutdown-related listeners such as
packages/webapp/socket_file.js, and it does not integrate the dev-mode parent watchdog (startCheckForLiveParent,boot.js:180) with the new hook runner. In dev mode, hooks taking longer than ~3s can therefore be pre-empted by the watchdogprocess.exit(1); in production builds (noMETEOR_PARENT_PID) theMETEOR_SHUTDOWN_TIMEOUT_MScap is the sole guard. Both can be handled in follow-up PRs.Validation
Manual validation against a fresh
meteor create --blazeapp.awaitMETEOR_SHUTDOWN_TIMEOUT_MS=2000)METEOR_SHUTDOWN_TIMEOUT_MS=0)process.exitSIGINT→ 130,SIGTERM→ 143