fix(cli): fall forward to a free port instead of failing veryfront dev - #3562
Conversation
|
Warning Review limit reached
Next review available in: 17 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: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe dev command now selects an available fallback port, starts related servers with the bound port, and returns it in ChangesDev server port fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant DevCommand
participant PortFallback
participant DevServer
participant MCPServer
participant Demo
Developer->>DevCommand: start development server
DevCommand->>PortFallback: findAvailablePort(requestedPort)
PortFallback-->>DevCommand: return boundPort
DevCommand->>DevServer: bind boundPort
DevCommand->>MCPServer: start with boundPort
DevCommand-->>Demo: return DevCommandResult with port
Demo->>Demo: build URL from result.port
Demo-->>Developer: display URL and open browser
**Suggested reviewers:** `kwakayama`, `ariskemper`
</details>
<!-- walkthrough_end -->
<!-- pre_merge_checks_walkthrough_start -->
<details>
<summary>🚥 Pre-merge checks | ✅ 5</summary>
<details>
<summary>✅ Passed checks (5 passed)</summary>
| Check name | Status | Explanation |
| :------------------------: | :------- | :----------------------------------------------------------------------------------------------------------------------------------- |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely describes the main change: falling forward to an available port when the requested port is occupied. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
</details>
</details>
<!-- pre_merge_checks_walkthrough_end -->
<!-- finishing_touch_checkbox_start -->
<details>
<summary>✨ Finishing Touches</summary>
<details>
<summary>📝 Generate docstrings</summary>
- [ ] <!-- {"checkboxId": "7962f53c-55bc-4827-bfbf-6a18da830691"} --> Create stacked PR
- [ ] <!-- {"checkboxId": "3e1879ae-f29b-4d0d-8e06-d12b7ba33d98"} --> Commit on current branch
</details>
<details>
<summary>🧪 Generate unit tests (beta)</summary>
- [ ] <!-- {"checkboxId": "f47ac10b-58cc-4372-a567-0e02b2c3d479", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Create PR with unit tests
- [ ] <!-- {"checkboxId": "6ba7b810-9dad-11d1-80b4-00c04fd430c8", "radioGroupId": "utg-output-choice-group-unknown_comment_id"} --> Commit unit tests in branch `fix/dx-20260811-0742-8`
</details>
</details>
<!-- finishing_touch_checkbox_end -->
<!-- tips_start -->
---
<sub>Comment `@coderabbitai help` to get the list of available commands.</sub>
<!-- tips_end -->
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92460e8428
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`veryfront dev` hard-failed when port 3000 was taken, with an error whose suggestion was the generic "Check initialization requirements and dependencies". The getting-started docs tell readers to run a bare `veryfront dev`, and never mention `--port`, so a doc-only reader had no way forward - on the single most contended port on a developer machine. Probe forward from the requested port for the first free one (10 attempts), announcing the switch. Once the whole range is taken, raise PORT_IN_USE, whose suggestion now names `veryfront dev --port <number>`. The scan probes ports by binding and releasing rather than by retrying startDevServer: a failed DevServer.start() has already registered file watchers and reload subscriptions that only stop() releases, so retrying it would leak a watcher set per busy port.
Two review findings on the port fallback, plus the generated API reference the error-registry edit invalidated. `devCommand` fell forward to a free port but `DevCommandResult` never exposed which one it chose, so `veryfront demo` kept printing and opening http://localhost:3000 - the address of the process that caused the collision in the first place. Return the bound port and have demo use it. `findAvailablePort` scanned `requestedPort + attempts` unconditionally, so a request near the top of the range eventually probed 65536, which a runtime rejects as an invalid port rather than as an address in use - letting that raw error escape instead of the promised PORT_IN_USE. Stop the scan at the last valid TCP port. An out-of-range `--port` is still probed, so the runtime's own complaint about the value the user passed still reaches them. Also regenerates docs/api-reference/veryfront/errors.md, whose source line pins shifted when PORT_IN_USE gained its suggestion.
2eee886 to
d13e8a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@cli/commands/dev/command.ts`:
- Around line 152-181: Add focused integration regression tests at
cli/commands/dev/command.ts lines 152-181 and cli/commands/demo/demo.ts lines
423-441. In the dev command test, force port fallback and assert startDevServer
receives the selected port and DevCommandResult.port returns it; in the demo
test, provide a non-3000 DevCommandResult.port and assert both displayed and
browser URLs use that port.
- Around line 160-177: Update the startup flow around startDevServer so
resources registered before HTTP binding are cleaned up when startup rejects
before returning a DevServer. Ensure the catch path invokes the available
partial-startup cleanup mechanism before rethrowing or converting the error,
while preserving the existing isPortInUseError handling and PORT_IN_USE
propagation.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ec56e1b-8cc8-4527-8a04-7dbf79c97621
📒 Files selected for processing (7)
cli/commands/demo/demo.tscli/commands/dev/command.tscli/commands/dev/dev.test.tscli/commands/dev/port-fallback.test.tscli/commands/dev/port-fallback.tsdocs/api-reference/veryfront/errors.mdsrc/errors/error-registry/server.ts
…iring CodeRabbit asked for regressions on the port the dev server actually binds: the utility tests proved the scan picks a free port, but nothing proved the command hands that port to the server and reports it, or that the demo shows and opens it. Both call sites needed a seam to test through. `startDevServerOnFreePort` takes the server start as a callback, so a test can force a real fall-forward by holding a port and assert the server input, the reported port, and the lost-bind-race PORT_IN_USE mapping. The demo's dev step moves to its own module so its URLs can be checked without booting a server, a browser, or stdin. Both regressions were confirmed red first: passing the requested port to `start` fails the two command tests, and hardcoding `localhost:3000` in the demo step fails the demo test.
… port Found during a DX dogfood walk of the developer journey. Running `veryfront dev` a second time in the same project, while the first server is still up, destroyed the shared `.cache` — the original walk saw 432 compiled `.mjs` files go to 0, forcing the running server to recompile everything on its next request. `cli/commands/dev/handler.ts` cleared the caches unconditionally, before it even called `devCommand`. The cache directory (`getCacheBaseDir()`, i.e. `<project>/.cache`) is shared by every dev server rooted at the same project, so the destructive step ran for a process that had no claim on the dev port. Move the clear into `devCommand`, behind a probe on the resolved port. A taken port is the signal that another dev server is already serving this project and still needs the modules it compiled, so the clear is skipped: the second `veryfront dev` falls forward to a free port (#3562) and starts on a cache it did not just destroy. A free port still clears, so the stale-module behaviour the clear exists for is unchanged. `clearLocalCaches` is opt-in, so `veryfront demo`'s programmatic `devCommand` call keeps its current behaviour of never clearing. `clearLocalCachesIfPortFree` takes its clear and probe as parameters, the same seam `startDevServerOnFreePort` uses, so the decision is testable without booting a dev server. Regression tests in cli/commands/dev/dev-cache-guard.integration.test.ts, all three confirmed red first against an unconditional clear: - a real held socket makes the probe report taken, and the seeded MDX-ESM and HTTP bundle entries survive; - a released port still clears them; - `handleDevCommand` driven against an unbindable port aborts inside devCommand with the seeded entries intact, which locks the handler ordering that caused the bug.
…ind fails DevServer.start() registers the file watchers, both ReloadNotifier subscriptions and the HMR external broadcast source before it binds the HTTP port. Callers only ever receive the instance after start() resolves — startDevServer() constructs it, awaits start() and returns it — so a bind failure dropped the half-built instance with no handle and left nobody able to call stop(). Every registration then survived for the life of the process. The leaked broadcast source is the worst of the three: it decrements a process-wide counter, and while it is held HMRHandler skips its own broadcast, so a single failed start could silently disable HMR for the dev server that starts next. Reachable from the probe-then-bind race in `veryfront dev`, and from any adapter.serve() failure that is not a port collision (bad bind address, permission denied on a privileged port). start() now routes failures through stop() and rethrows. stop() is already null-safe at every step, so it tears down however far start() got, and it remains the single teardown path — a new registration added to start() cannot drift out of sync with a second cleanup list. Fixing it here rather than in startDevServer() also covers callers that construct DevServer directly. This matches the release-before-rethrow the codebase already uses in bootstrap.ts (orchestrateOrDisposeFS) and in the Deno http-server adapter. stop() now clears the release handles after calling them, so the double-stop this introduces cannot decrement the broadcast counter twice. Surfaced by CodeRabbit on #3562 and declined there as out of scope.
…ind fails DevServer.start() registers the file watchers, both ReloadNotifier subscriptions and the HMR external broadcast source before it binds the HTTP port. Callers only ever receive the instance after start() resolves — startDevServer() constructs it, awaits start() and returns it — so a bind failure dropped the half-built instance with no handle and left nobody able to call stop(). Every registration then survived for the life of the process. The leaked broadcast source is the worst of the three: it decrements a process-wide counter, and while it is held HMRHandler skips its own broadcast, so a single failed start could silently disable HMR for the dev server that starts next. Reachable from the probe-then-bind race in `veryfront dev`, and from any adapter.serve() failure that is not a port collision (bad bind address, permission denied on a privileged port). start() now routes failures through stop() and rethrows. stop() is already null-safe at every step, so it tears down however far start() got, and it remains the single teardown path — a new registration added to start() cannot drift out of sync with a second cleanup list. Fixing it here rather than in startDevServer() also covers callers that construct DevServer directly. This matches the release-before-rethrow the codebase already uses in bootstrap.ts (orchestrateOrDisposeFS) and in the Deno http-server adapter. stop() now clears the release handles after calling them, so the double-stop this introduces cannot decrement the broadcast counter twice. Surfaced by CodeRabbit on #3562 and declined there as out of scope.
…ind fails DevServer.start() registers the file watchers, both ReloadNotifier subscriptions and the HMR external broadcast source before it binds the HTTP port. Callers only ever receive the instance after start() resolves — startDevServer() constructs it, awaits start() and returns it — so a bind failure dropped the half-built instance with no handle and left nobody able to call stop(). Every registration then survived for the life of the process. The leaked broadcast source is the worst of the three: it decrements a process-wide counter, and while it is held HMRHandler skips its own broadcast, so a single failed start could silently disable HMR for the dev server that starts next. Reachable from the probe-then-bind race in `veryfront dev`, and from any adapter.serve() failure that is not a port collision (bad bind address, permission denied on a privileged port). start() now routes failures through stop() and rethrows. stop() is already null-safe at every step, so it tears down however far start() got, and it remains the single teardown path — a new registration added to start() cannot drift out of sync with a second cleanup list. Fixing it here rather than in startDevServer() also covers callers that construct DevServer directly. This matches the release-before-rethrow the codebase already uses in bootstrap.ts (orchestrateOrDisposeFS) and in the Deno http-server adapter. stop() now clears the release handles after calling them, so the double-stop this introduces cannot decrement the broadcast counter twice. Surfaced by CodeRabbit on #3562 and declined there as out of scope.
Found by a DX dogfood walk of https://veryfront.com/docs/code/getting-started/create-project against published CLI 0.1.1228.
Symptom
The create-project page says: "Run the dev server:
veryfront dev. Open http://localhost:3000." It never mentions--port— neither does the installation page. If anything already holds port 3000, that literal instruction dead-ends:No fallback to a free port, and the suggestion line is the generic initialization boilerplate. The
--portflag exists, but onlyveryfront dev --helpreveals it — a doc-only reader is stuck, on the single most contended port on a developer machine.Reproduced against the local tree at
fe5d5b895(Release v0.1.1228), so it was not already fixed and unreleased.Root cause
devCommandcalledstartDevServeronce with the resolved port and translated any address-in-use failure intoINITIALIZATION_ERROR, whose registry suggestion is "Check initialization requirements and dependencies". There was no scan for a free port, and nothing in the error path named the flag that would resolve it.The framework already had the right error (
PORT_IN_USE) and the right precedent (the login callback server incli/auth/callback-server.tsscans forward overMAX_PORT_ATTEMPTS), butveryfront devused neither.Fix
cli/commands/dev/port-fallback.ts:findAvailablePortprobes forward from the requested port for the first free one (10 attempts), plus a runtime-portableisPortAvailable/isPortInUseError.devCommanduses the chosen port for the server, the MCP port, and the printed URL, and announces the switch:PORT_IN_USEis raised instead ofINITIALIZATION_ERROR, and its suggestion now names the flag:The scan probes ports by binding and releasing them rather than by retrying
startDevServer. That is deliberate: a failedDevServer.start()has already registered file watchers and reload subscriptions that onlystop()releases, so a retry loop around it would leak a watcher set per busy port. A lost probe/bind race is still caught and reported asPORT_IN_USE.PORT_IN_USEhad no callers before this change, so no other error surface shifts.Regression test
cli/commands/dev/port-fallback.test.ts— Deno BDD, alongside the code it covers.It lives in
veryfront-coderather thanveryfront-e2ebecause the bug reproduces entirely in-process: it needs a held TCP port and the CLI's port-selection logic, no browser, no deployment, no credentials. That keeps it in the pre-push gate.Coverage:
port-in-usewith a suggestion containing--port.isPortAvailableand skipped byfindAvailablePort, and the runtime's ownAddrInUseerror is recognised (guarding against a runtime error-shape change silently disabling the fallback).Verified red before the fix — with the pre-fix single-bind behaviour in place, 7 steps failed on exactly those assertions (fall-forward cases threw; the exhausted case reported
initialization-error).Verification
cli/commands,src/errors,src/server/dev-server: 227 passed, 0 failed.127.0.0.1:3000with a real listener: pre-fix produced theinitialization-errorabove; post-fix falls forward to 3001. With 3000–3009 all held, theport-in-useerror above is produced.Out of scope
The docs still do not mention
--portanywhere on the create-project or installation pages. This PR makes the flag discoverable from the error path; the doc side belongs toveryfront-docs.Review follow-up (2eee886)
Two findings from the Codex review, both real:
veryfront demorunsdevCommandand then printed and openedhttp://localhost:3000literally. Before the fallback existed a busy 3000 threw, so that line was unreachable; with the fallback it succeeds on 3001 and sends the user to the squatting process.DevCommandResultnow carriesport— the port actually bound, from both exit paths including proxy mode — anddemo.tsbuilds its URL from it.PORT_IN_USE. The scan now stops at 65535 and reports the real last port scanned. An out-of-range--portis still probed rather than clamped, so the user hears the runtime's own complaint about the value they passed instead of a misleading "ports 70000-65535 are all in use".Both behaviours are covered by new cases in
port-fallback.test.ts.docs/api-reference/veryfront/errors.mdis regenerated in the same commit: its source line pins shifted whenPORT_IN_USEgained its suggestion.Summary by CodeRabbit