fix(dev): probe both loopback families for port availability, and de-flake its test - #3650
Merged
Conversation
`isPortAvailable` bound only 127.0.0.1, so a port held on ::1 probed as free. That is not a hypothetical family: `veryfront dev` serves the app through an adapter defaulting to `LOCALHOST.IPV4`, but starts its MCP server (`--port + 2`) through the Node adapter, which defaults to the name `localhost` and so resolves to `::1` first on any dual-stack host. With one dev server holding [::1]:3002 for MCP, a second one announced "Port 3000 is in use, using 3002 instead" and bound its HTTP server to 127.0.0.1:3002 - a port the first instance had already reserved. It did not hard-fail only because the two listeners landed on different families, which is luck rather than correctness. Probe 127.0.0.1 and ::1 both, and treat a port as available only when neither is holding it. The literal addresses are used rather than the name `localhost` because a listen on a name binds just the first address it resolves to, leaving the other family unchecked exactly as before. The node:net branch had the same IPv4-only bias and is fixed with it. A family the host does not have is skipped rather than counted as a collision, so probing more families cannot make a genuinely free port report as busy: `isAddressFamilyUnavailableError` tells EADDRNOTAVAIL / EAFNOSUPPORT apart from EADDRINUSE, and an IPv4-only container still falls forward normally.
"isPortAvailable ... accepts a port nothing is holding" reserved an ephemeral port, released it, and asserted it was still free. Nothing can hold a port open and leave it bindable at the same time, so that gap is open to any other process - and CI runs ~30 jobs against one host. It ejected PR #3597, an unrelated CSS-hint change, from the merge queue. This is a test-isolation defect, not a symptom of the IPv4-only probe fixed in the previous commit: the assertion is unsound whatever the product does. Retry on a freshly reserved port instead. A probe that wrongly rejects free ports rejects all 25 of them and still fails the test, so nothing is softened - only the assumption that one particular port survives the gap is gone. Verified by reproducing the race deterministically (claiming the released port inside the window makes the old body fail and the new body pass), then running the file 60x against a host holding 13000 of its 16384 ephemeral ports with 8 processes cycling more: 60/60 green.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4a9e80357
ℹ️ 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".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPort fallback probing now checks IPv4 and IPv6 loopback addresses in Deno and Node. It skips unsupported address families, detects occupied ports across families, retries released-port checks, and validates error classification. ChangesPort probing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant isPortAvailable
participant RuntimeProbe
participant LoopbackAddress
isPortAvailable->>RuntimeProbe: probe IPv4 and IPv6 loopback hosts
RuntimeProbe->>LoopbackAddress: bind and release port
LoopbackAddress-->>RuntimeProbe: success or address-family error
RuntimeProbe-->>isPortAvailable: available or occupied
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Review catch on #3650, and it is the same defect this PR exists to remove. The new classifier test provoked its error by binding `::2` and asserting the bind fails - swapping "an arbitrary port stays free" for "an arbitrary address stays unbindable". Both are assumptions about ambient host state. On a host where `::2` is assigned, or on Linux with `net.ipv6.ip_nonlocal_bind=1`, the bind succeeds and the test fails against a correct implementation. Construct the error shapes instead. `Deno.errors.AddrNotAvailable` is the runtime's own constructor rather than a hand-rolled Error with a spoofed name, so a rename in Deno fails the test loudly instead of letting it drift away from what the probe actually catches. Constructing also reaches coverage a live bind cannot: the Node `code` shapes (EADDRNOTAVAIL, EAFNOSUPPORT) are unreachable from a bind on a Deno host, and the message-only path now has explicit cases for both the macOS and Linux wordings. Preferred over skipping when the host permits the bind: a skip stops asserting on exactly the hosts where the behaviour is most interesting, and nothing reports that it went quiet. Mutation-checked - dropping any one of the name, code, or message branches of the classifier is caught by at least one of these assertions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@cli/commands/dev/port-fallback.test.ts`:
- Around line 28-46: Update listenOnLoopback so it returns null only when
isAddressFamilyUnavailableError(error) is true; rethrow all other bind errors,
including those not identified by isPortInUseError, so permission and resource
failures propagate.
🪄 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: 641037d5-bab7-45a2-8d72-345b3cfa2646
📒 Files selected for processing (2)
cli/commands/dev/port-fallback.test.tscli/commands/dev/port-fallback.ts
…ipping Second review catch on #3650, same theme as the first. `listenOnLoopback` returned null for every bind error that was not a port collision, so a permission failure or a resource limit silently turned "skips a port held on IPv6 only" into a no-op that still reported green. Skip only for a missing address family - the one condition where there is genuinely nothing to test - and rethrow everything else. Verified: a hostname that fails to resolve used to return null and skip; it now propagates and fails the test. The real IPv4-only skip path is unchanged.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two separate defects in
cli/commands/dev/port-fallback.ts, one commit each.1. Product:
isPortAvailableprobed only IPv4isPortAvailablebound only127.0.0.1, so a port held on::1probed as free.That is not a hypothetical family.
veryfront devruns two listeners and they do not land on the same one:--port, 3000)DenoHttpServer.serve(src/platform/compat/http/deno-server.ts:59)LOCALHOST.IPV4127.0.0.1--port + 2, 3002)src/platform/adapters/runtime/node/http-server.ts:573), reached viacli/mcp/server.ts:224which passes no hostname"localhost"::1first on any dual-stack hostObserved against a real install of
veryfront@0.1.1231-rc.11370: with instance A holding[::1]:3002for MCP, a secondveryfront devreported! Port 3000 is in use, using 3002 insteadand bound its HTTP server to127.0.0.1:3002— a port instance A had already reserved. It did not hard-fail only because the two listeners landed on different families. That is luck, not correctness.Fix: probe
127.0.0.1and::1both; a port is available only when neither holds it. The literal addresses are used rather than the namelocalhost, because a listen on a name binds only the first address it resolves to — which would leave the other family unchecked exactly as before.The
node:netbranch had the same IPv4-only bias (it hardcodedhost: LOCALHOST.IPV4too) and is fixed with it.Inverse regression guarded: probing more families must not make a genuinely free port report busy.
isAddressFamilyUnavailableErrordistinguishesEADDRNOTAVAIL/EAFNOSUPPORT(this host has no such family — skip it) fromEADDRINUSE(a real collision), so an IPv4-only container still falls forward normally. Anything else still propagates, so an out-of-range--portkeeps surfacing the runtime's own complaint.2. Test isolation: the flake that ejected #3597
isPortAvailable ... accepts a port nothing is holdingreserved an ephemeral port, released it, and asserted it was still free. Nothing can hold a port open and leave it bindable at the same time, so that gap is open to any other process — and CI runs ~30 jobs against one host. It ejected #3597, an unrelated CSS-hint change, from the merge queue.This is a test-isolation defect, distinct from the product bug: the assertion is unsound whatever the product does. Kept in its own commit.
Fix: retry on a freshly reserved port. Nothing is softened — a probe that wrongly rejects free ports rejects all 25 of them and still fails the test. Only the assumption that one particular port survives the gap is gone.
Verification
Values are not equal: actual true / expected false—isPortAvailableaccepting a port held on[::1].127.0.0.1:Pand[::1]:P+2held,findAvailablePort(P)now returnsP+1instead of colliding onP+2.deno lint,deno fmt --check,deno checkclean on both files.cli/unit suite: 271 passed, 0 failed.No existing test was weakened or removed.
Summary by CodeRabbit
Bug Fixes
Tests