fix(dev): keep serving during a hard restart using SO_REUSEPORT - #1404
Conversation
commit: |
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe dev command now supports reuse-port handovers during forked restarts, waits for replacement forks to serve, and defers cascading restart handling until readiness. Listener binding adds handover and reuse-port probing. ForkPool exposes serving-aware fork controls and per-fork listen overrides. Dev context and lock acquisition carry the outgoing process PID for takeover. Unit tests cover listener, fork lifecycle, restart hooks, and lock behavior. Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxt-cli/src/dev/pool.ts (1)
208-215: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winOutgoing fork stays
serving: true, so its shutdown exit code can kill the whole session.
promote()setsservingbut nothing ever clears it. After a handover,dev.tscallsclosePrevious()on the previously promoted fork; if that fork exits with a non-zero code while shutting down (rather than being reaped cleanly by the signal), this handler callsprocess.exit(errorCode)and takes down the dev session even though the new fork is now serving. Clear the flag when the fork is deliberately killed.🐛 Clear `serving` on deliberate shutdown
private killFork(fork: PooledFork, signal: NodeJS.Signals | number = 'SIGTERM'): Promise<void> { const wasAlive = fork.state !== 'dead' && !!fork.process && fork.process.exitCode === null fork.state = 'dead' + // A fork we are shutting down on purpose must not end the session, even if + // it exits non-zero on the way out. + fork.serving = false if (fork.process) {🤖 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/nuxt-cli/src/dev/pool.ts` around lines 208 - 215, Update the deliberate shutdown path used by closePrevious() to clear the promoted fork’s serving flag before killing it. Ensure pooledFork.serving is false before the close handler evaluates it, while preserving the existing process.exit behavior for unexpected crashes of active serving forks.
🧹 Nitpick comments (1)
packages/nuxt-cli/src/dev/listen.ts (1)
336-359: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueProbe servers are closed but not awaited, and a failed probe can leave the first socket bound briefly.
server.close()is asynchronous; the cached result resolves before the ephemeral ports are actually released. Harmless in practice (ephemeral ports, process-lifetime cache), but awaiting the closes makes the probe self-contained and avoids surprising a test that asserts on open handles.♻️ Await the probe teardown
finally { - for (const server of [first, second]) { - if (server.listening) { - server.close() - } - } + await Promise.all([first, second].map(server => new Promise<void>((resolve) => { + if (!server.listening) { + resolve() + return + } + server.close(() => resolve()) + }))) }🤖 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/nuxt-cli/src/dev/listen.ts` around lines 336 - 359, Update the finally block in isReusePortSupported to await completion of server.close() for each listening probe server before the cached promise resolves, including the failed-bind path. Preserve cleanup for both first and second servers and keep non-listening servers skipped.
🤖 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/nuxt-cli/src/commands/dev.ts`:
- Around line 251-261: Update the failed-handover handling around the
`onRestart(restart)` call so it re-arms the existing restart subscription
instead of registering new process-level and `devServer` listeners. Adjust
`initialize` and its returned restart-handling API as needed to remove or reuse
prior handlers while preserving restart behavior after a failed handover.
- Around line 204-216: Serialize restartWithFork invocations with a single
in-flight promise so concurrent fork restart/rejection messages and
shortcut-triggered restarts cannot overlap. Update restartWithFork and its
callers to reuse or await the active restart operation, while preserving the
existing handover, closeCurrent, and currentPid sequencing for each restart.
- Around line 226-249: Guard the pool.getFork() acquisition inside
restartWithFork with the same recovery handling used when fork.serving fails.
Catch readiness or IPC-send rejections, retain the current server when available
(especially in non-handover mode), and return without allowing errors from the
unawaited restartWithFork or onRestart paths to become unhandled rejections.
---
Outside diff comments:
In `@packages/nuxt-cli/src/dev/pool.ts`:
- Around line 208-215: Update the deliberate shutdown path used by
closePrevious() to clear the promoted fork’s serving flag before killing it.
Ensure pooledFork.serving is false before the close handler evaluates it, while
preserving the existing process.exit behavior for unexpected crashes of active
serving forks.
---
Nitpick comments:
In `@packages/nuxt-cli/src/dev/listen.ts`:
- Around line 336-359: Update the finally block in isReusePortSupported to await
completion of server.close() for each listening probe server before the cached
promise resolves, including the failed-bind path. Preserve cleanup for both
first and second servers and keep non-listening servers skipped.
🪄 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 Plus
Run ID: c22c6610-4d3b-4908-b2c7-7d5271307ee6
📒 Files selected for processing (9)
packages/nuxt-cli/src/commands/dev.tspackages/nuxt-cli/src/dev/index.tspackages/nuxt-cli/src/dev/listen.tspackages/nuxt-cli/src/dev/pool.tspackages/nuxt-cli/src/dev/utils.tspackages/nuxt-cli/src/utils/lockfile.tspackages/nuxt-cli/test/unit/listen.spec.tspackages/nuxt-cli/test/unit/lockfile.spec.tspackages/nuxt-cli/test/unit/pool.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxt-cli/src/dev/pool.ts (1)
206-219: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject
readywhen a fork exits before readiness.
getFork()awaitsfork.readyat Line 95, but this promise is only rejected onerror. A child can emitclosewithouterror, leaving restart callers and warming logic blocked forever while the fork is removed from the pool. Rejectreadyfrom theclosehandler before removing the fork.Proposed fix
childProc.on('close', (errorCode) => { + readyReject(new Error('Dev server fork exited before sending fork-ready.')) if (pooledFork.serving && errorCode) { // Active fork crashed process.exit(errorCode) } this.removeFork(pooledFork) })🤖 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/nuxt-cli/src/dev/pool.ts` around lines 206 - 219, Update the childProc close handler near the existing error handler to reject pooledFork.ready when the fork closes before becoming ready, including clean closes without an error event. Perform this rejection before this.removeFork(pooledFork), while preserving the existing serving-fork exit behavior.
🤖 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/nuxt-cli/src/dev/index.ts`:
- Around line 191-235: Update restart() in createRestartHook to remove the
pending source 'restart' listener with source.off('restart', restart) when
handling any restart, including error-triggered restarts. Extend coverage as
needed to verify repeated error-triggered re-arming does not accumulate source
listeners.
---
Outside diff comments:
In `@packages/nuxt-cli/src/dev/pool.ts`:
- Around line 206-219: Update the childProc close handler near the existing
error handler to reject pooledFork.ready when the fork closes before becoming
ready, including clean closes without an error event. Perform this rejection
before this.removeFork(pooledFork), while preserving the existing serving-fork
exit behavior.
🪄 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 Plus
Run ID: 07364a55-c6e3-4255-9b9d-9b278bb6665a
📒 Files selected for processing (5)
packages/nuxt-cli/src/commands/dev.tspackages/nuxt-cli/src/dev/index.tspackages/nuxt-cli/src/dev/pool.tspackages/nuxt-cli/test/unit/pool.spec.tspackages/nuxt-cli/test/unit/restart-hook.spec.ts
💤 Files with no reviewable changes (1)
- packages/nuxt-cli/test/unit/pool.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxt-cli/src/commands/dev.ts
1 participant
🔗 Linked issue
📚 Description
rather than kill a fork, then start again and hope that the port is the same, this keeps the port, and hands it across to the new fork