fix(nuxi): timeout NuxtDevServer.close() to avoid dev-restart deadlock#1328
fix(nuxi): timeout NuxtDevServer.close() to avoid dev-restart deadlock#1328mttzzz wants to merge 6 commits into
Conversation
When a nitro plugin holds a long-lived connection (Bull `BLPOP`/`BRPOPLPUSH`, Postgres `LISTEN`, WebSocket etc.) and registers a close hook, `nitroApp.close()` awaits forever for that handle to drain, and the dev server stays in a permanent "Restarting Nuxt..." state on `nuxt.config.ts` changes. Cap the wait with a small timeout (3s by default, override via `NUXT_DEV_CLOSE_TIMEOUT_MS`). The new instance then proceeds to bind and the GC reaps the orphan Nuxt instance. Refs nuxt/nuxt#32928
commit: |
|
Warning Review limit reached
More reviews will be available in 3 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds DEFAULT_CLOSE_TIMEOUT_MS and closeWithTimeout(closer, timeoutMs) which races a provided closer against a timeout, clears the timer when settled, and swallows closer rejections and sync throws. NuxtDevServer.close() now no-ops when no Estimated code review effort🎯 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1328 +/- ##
=======================================
Coverage ? 52.32%
=======================================
Files ? 49
Lines ? 1227
Branches ? 341
=======================================
Hits ? 642
Misses ? 479
Partials ? 106 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Actionable comments posted: 0 |
Refactor the timeout/race logic out of `close()` into a pure `closeWithTimeout(closer, timeoutMs)` helper so we can unit-test the behaviour in isolation. Adds 5 specs: - fast resolve passes through - never-resolving closer is unblocked by the timeout (the simulated Bull `BLPOP` / Postgres `LISTEN` deadlock case) - closer rejection is swallowed so restart can proceed - the safety timer is cleared on fast close (no leftover handle) - a non-zero default constant is exposed The override env var (`NUXT_DEV_CLOSE_TIMEOUT_MS`) and the default 3-second cap stay unchanged from the original PR.
|
Added a unit test for the extracted |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/nuxi/test/unit/close-with-timeout.spec.ts (2)
43-48: 💤 Low valueConsider verifying timer cleanup in the rejection test.
Test 5 (lines 50-55) verifies that timers are cleaned up after a fast close, but this rejection test doesn't include a similar check. Adding
expect(vi.getTimerCount()).toBe(0)would ensure consistent verification that the timer is cleared in the.finally()block even when the closer rejects.🧹 Proposed enhancement
it('swallows closer rejections so restart can proceed', async () => { const closer = vi.fn().mockRejectedValue(new Error('boom')) const result = closeWithTimeout(closer, 1000) await vi.advanceTimersByTimeAsync(0) await expect(result).resolves.toBeUndefined() + expect(vi.getTimerCount()).toBe(0) })🤖 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/nuxi/test/unit/close-with-timeout.spec.ts` around lines 43 - 48, The rejection test for closeWithTimeout should also verify the timers are cleaned up: after calling closeWithTimeout(closer, 1000) and advancing timers (vi.advanceTimersByTimeAsync(0)), add an assertion like expect(vi.getTimerCount()).toBe(0) to confirm the timer created by closeWithTimeout is cleared in its .finally() block even when the closer rejects; locate the test using the closer mock and closeWithTimeout invocation and add the timer count expectation after awaiting the result resolution.
9-56: ⚡ Quick winConsider adding a test for closer that resolves after a delay but before timeout.
The test suite covers immediate resolution (0ms) and never-resolving closers, but not the scenario where the closer wins the race after a non-zero delay (e.g., closer takes 500ms when timeout is 1000ms). This would verify that the
.finally()timer cleanup works correctly when the closer wins after some time has passed.🧪 Proposed additional test case
it('resolves when closer wins after a delay and clears the timer', async () => { const closer = vi.fn(() => new Promise<void>((resolve) => { setTimeout(resolve, 500) })) const result = closeWithTimeout(closer, 1000) // Advance to when closer should resolve await vi.advanceTimersByTimeAsync(500) await expect(result).resolves.toBeUndefined() expect(closer).toHaveBeenCalledOnce() // Timer should be cleaned up even though closer won after delay expect(vi.getTimerCount()).toBe(0) })🤖 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/nuxi/test/unit/close-with-timeout.spec.ts` around lines 9 - 56, Add a test that verifies closeWithTimeout correctly handles a closer that resolves after a delay but before the timeout: create a closer function (used by closeWithTimeout) that resolves via setTimeout after e.g. 500ms, call closeWithTimeout(closer, 1000), advance timers to 500ms with vi.advanceTimersByTimeAsync, assert the returned promise resolves, assert the closer was called (closer/vi.fn), and finally assert vi.getTimerCount() is 0 to ensure the cleanup in closeWithTimeout's finally removed the timeout.
🤖 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 `@packages/nuxi/src/dev/utils.ts`:
- Around line 88-90: The call to closer() inside closeWithTimeout can throw
synchronously so the current .catch() won't handle it; change the invocation to
wrap closer with Promise.resolve().then(closer) (e.g., use
Promise.resolve().then(() => closer()) or Promise.resolve().then(closer)) when
building the Promise.race so both synchronous throws and async rejections from
closer are consistently caught and swallowed by the existing .catch(),
preserving restart safety.
- Around line 530-533: The timeout value derived from
process.env.NUXT_DEV_CLOSE_TIMEOUT_MS is not validated and using Number(...) ||
DEFAULT_CLOSE_TIMEOUT_MS can still allow negative, zero, NaN or Infinity to
sneak through; update the call site that invokes closeWithTimeout (the lambda
calling this.#currentNuxt!.close()) to first parse the env value into a numeric
variable (e.g., parsedTimeout), check Number.isFinite(parsedTimeout) and
parsedTimeout > 0, and only then pass parsedTimeout to closeWithTimeout;
otherwise fallback to DEFAULT_CLOSE_TIMEOUT_MS so closeWithTimeout always
receives a finite positive timeout.
---
Nitpick comments:
In `@packages/nuxi/test/unit/close-with-timeout.spec.ts`:
- Around line 43-48: The rejection test for closeWithTimeout should also verify
the timers are cleaned up: after calling closeWithTimeout(closer, 1000) and
advancing timers (vi.advanceTimersByTimeAsync(0)), add an assertion like
expect(vi.getTimerCount()).toBe(0) to confirm the timer created by
closeWithTimeout is cleared in its .finally() block even when the closer
rejects; locate the test using the closer mock and closeWithTimeout invocation
and add the timer count expectation after awaiting the result resolution.
- Around line 9-56: Add a test that verifies closeWithTimeout correctly handles
a closer that resolves after a delay but before the timeout: create a closer
function (used by closeWithTimeout) that resolves via setTimeout after e.g.
500ms, call closeWithTimeout(closer, 1000), advance timers to 500ms with
vi.advanceTimersByTimeAsync, assert the returned promise resolves, assert the
closer was called (closer/vi.fn), and finally assert vi.getTimerCount() is 0 to
ensure the cleanup in closeWithTimeout's finally removed the timeout.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fddd0cdc-40c8-4b51-91cd-5e88d3e911f0
📒 Files selected for processing (2)
packages/nuxi/src/dev/utils.tspackages/nuxi/test/unit/close-with-timeout.spec.ts
| await closeWithTimeout( | ||
| () => this.#currentNuxt!.close(), | ||
| Number(process.env.NUXT_DEV_CLOSE_TIMEOUT_MS) || DEFAULT_CLOSE_TIMEOUT_MS, | ||
| ) |
There was a problem hiding this comment.
Validate the env timeout before using it.
Number(...) || DEFAULT_CLOSE_TIMEOUT_MS does not guard against negative or non-finite values. Validating for finite positive numbers avoids accidental immediate/invalid timeout behavior.
Suggested fix
+ const envTimeout = Number(process.env.NUXT_DEV_CLOSE_TIMEOUT_MS)
+ const timeoutMs = Number.isFinite(envTimeout) && envTimeout > 0
+ ? envTimeout
+ : DEFAULT_CLOSE_TIMEOUT_MS
await closeWithTimeout(
() => this.#currentNuxt!.close(),
- Number(process.env.NUXT_DEV_CLOSE_TIMEOUT_MS) || DEFAULT_CLOSE_TIMEOUT_MS,
+ timeoutMs,
)📝 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.
| await closeWithTimeout( | |
| () => this.#currentNuxt!.close(), | |
| Number(process.env.NUXT_DEV_CLOSE_TIMEOUT_MS) || DEFAULT_CLOSE_TIMEOUT_MS, | |
| ) | |
| const envTimeout = Number(process.env.NUXT_DEV_CLOSE_TIMEOUT_MS) | |
| const timeoutMs = Number.isFinite(envTimeout) && envTimeout > 0 | |
| ? envTimeout | |
| : DEFAULT_CLOSE_TIMEOUT_MS | |
| await closeWithTimeout( | |
| () => this.#currentNuxt!.close(), | |
| timeoutMs, | |
| ) |
🤖 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/nuxi/src/dev/utils.ts` around lines 530 - 533, The timeout value
derived from process.env.NUXT_DEV_CLOSE_TIMEOUT_MS is not validated and using
Number(...) || DEFAULT_CLOSE_TIMEOUT_MS can still allow negative, zero, NaN or
Infinity to sneak through; update the call site that invokes closeWithTimeout
(the lambda calling this.#currentNuxt!.close()) to first parse the env value
into a numeric variable (e.g., parsedTimeout), check
Number.isFinite(parsedTimeout) and parsedTimeout > 0, and only then pass
parsedTimeout to closeWithTimeout; otherwise fallback to
DEFAULT_CLOSE_TIMEOUT_MS so closeWithTimeout always receives a finite positive
timeout.
Calling `closer()` directly skipped the `.catch` chain if the function threw synchronously, which would then reject `closeWithTimeout` and still abort the restart it was meant to protect. Wrap the invocation in `Promise.resolve().then(closer)` so both sync throws and async rejections funnel through `.catch()` and the safety net stays consistent. Also add a test that proves the sync-throw path resolves, and a small test for `NuxtDevServer.close()` early-return when no Nuxt instance has been initialised yet (covers the only previously-uncovered line in the patch). Addresses CodeRabbit review on nuxt#1328.
|
Actionable comments posted: 0 |
The remaining uncovered line was `await closeWithTimeout(...)` inside `NuxtDevServer.close()`. Reaching it from a unit test would require mocking the private `#currentNuxt` field, which JS-private semantics forbid. The helper itself is already exhaustively unit-tested, so the inline call is a thin delegation we can safely exclude from coverage. Brings patch coverage to 100%.
2 participants
Issue
Refs nuxt/nuxt#32928
The Nuxt dev server can enter a permanent "Restarting Nuxt..." state after a
nuxt.config.tsedit (or other change that triggers a full reload) when any nitro plugin holds a long-lived background handle.NuxtDevServer.close()awaitscurrentNuxt.close()which in turn awaits every registeredclosehook. If any plugin owns a connection that is blocked on a server response (BullBLPOP/BRPOPLPUSH, PostgresLISTEN, native WebSocket, anioredis-shared client, etc.) the close hook never resolves — and the restart deadlocks.This is fairly easy to hit in real apps: queues (BullMQ/Bull), realtime endpoints, background pollers all rely on connections that don't drain in a few hundred ms.
Change
Wrap the inner
close()in aPromise.raceagainst a 3-second timeout. After the deadline the restart proceeds; the new Nuxt instance binds the port (the OS reclaims the bind from the orphaned process) and the lingering handles are GC'd along with the dropped reference to the old Nuxt instance.The timeout is overridable via
NUXT_DEV_CLOSE_TIMEOUT_MSfor projects that legitimately need more grace (e.g. heavy in-flight migrations on close).Behaviour
close()resolves in milliseconds, timer is cleared, no leftover handles in the event loop.timeoutMs, then proceed with the restart. Equivalent to the user pressing Ctrl-C after the deadlock and re-runningbun dev, only automatic.Trade-offs
This is intentionally a safety net rather than a root-cause fix — the real fix lives in the user's plugin (
queue.close(true),await server.unref()on a websocket, etc.). But the failure mode is hard to diagnose for users (CLI says "Restarting Nuxt..." with no further output) and the timeout makes the dev experience consistent with what most people expect from an HMR-driven workflow.Tested
Reproduced locally with a Nuxt 4.4.6 + 6 Bull workers + Postgres LISTEN setup that previously deadlocked indefinitely on every
nuxt.config.tssave. With the patch, restart completes in 6-10s consistently.