Add ipv6_v6only config option for consistent dual-stack behavior - #3060
Add ipv6_v6only config option for consistent dual-stack behavior#3060pranjalm37 wants to merge 8 commits into
Conversation
Binding an IPv6 host currently behaves inconsistently depending on worker count: single-worker mode delegates socket creation to asyncio.loop.create_server(), which unconditionally forces IPV6_V6ONLY=True; multi-worker mode creates its own socket in Config.bind_socket() and never touches that option, so it inherits the OS default (dual-stack on Linux). A server started with --host '::' silently drops IPv4 connectivity in the (default) single-worker case but accepts it with --workers 2+. Add an explicit `ipv6_v6only: bool | None = None` Config option (and --ipv6-v6only/--no-ipv6-v6only CLI flag) that, when set, is applied consistently in both code paths: - bind_socket() now sets IPV6_V6ONLY explicitly when ipv6_v6only is not None (used directly in multi-worker mode). - The single-worker path in Server.startup() now also routes through bind_socket() (passing the socket via `sock=` instead of `host=`/`port=`) whenever ipv6_v6only is explicitly set, since asyncio's create_server() would otherwise silently override any pre-set socket option. - bind_socket() gained a `log` parameter so the single-worker path can keep using its own _log_started_message() and avoid a duplicate startup log line. Left at None (the default) it's a no-op: neither code path's existing default behavior changes, so this is purely additive. Fixes Kludex#2945
|
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:
📝 WalkthroughWalkthroughThe change adds optional IPv6 ChangesIPv6 V6ONLY control
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as main()
participant Config
participant Server
participant Socket as IPv6 socket
CLI->>Config: pass ipv6_v6only
Config->>Socket: set IPV6_V6ONLY when configured
Server->>Config: bind_socket(log=False)
Config->>Socket: bind IPv6 socket
Server->>Socket: pass socket to asyncio.create_server
Possibly related PRs
🚥 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
🤖 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 `@docs/settings.md`:
- Line 48: Update the `--ipv6-v6only` documentation to remove the claim that
Linux defaults to dual-stack; describe the multi-worker behavior as inheriting
the OS-configurable default, which may be IPv6-only or dual-stack. Preserve the
existing explanation of single-worker behavior and the `--no-ipv6-v6only`
option.
In `@uvicorn/config.py`:
- Line 197: Move the ipv6_v6only parameter in Config.__init__ to after all
existing constructor parameters, preserving the current positional order so
arguments following port retain their original meanings.
In `@uvicorn/server.py`:
- Line 182: Update the explicit socket-binding path around
config.bind_socket(log=False) to catch SystemExit in addition to the existing
bind-failure handling; when binding fails after lifespan startup, await
self.lifespan.shutdown() and then re-raise the same SystemExit.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c72bbd94-521c-497c-a42e-56665516a29a
📒 Files selected for processing (6)
docs/settings.mdtests/test_config.pytests/test_server.pyuvicorn/config.pyuvicorn/main.pyuvicorn/server.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… docs - Config.__init__: move ipv6_v6only to the end of the parameter list instead of inserting it between port and uds. There's no keyword-only separator, so the insertion silently shifted every positional argument after it (e.g. Config(app, host, port, uds) would pass uds's value as ipv6_v6only instead). Caught independently by two automated reviewers. - Server.startup(): the explicit-bind path calls config.bind_socket(), which does sys.exit() on a bind OSError -- but the surrounding `except OSError` doesn't catch that SystemExit, so an occupied IPv6 port skipped lifespan.shutdown(), unlike the pre-existing failure path. Catch SystemExit too, run lifespan cleanup, then re-raise. - docs/settings.md: don't overstate "dual-stack on Linux" as a hard guarantee -- it's controlled by the net.ipv6.bindv6only sysctl and can be configured either way. - tests: extract the ad-hoc _has_ipv6() helper from test_main.py into a shared tests/utils.py::has_ipv6(), and use it to skip the new IPv6 tests (in test_config.py and test_server.py) in environments without IPv6 support, instead of hard-failing there.
…host Reproduced on real Linux (Docker): binding specifically to the loopback address '::1' silently forces IPV6_V6ONLY=True regardless of the requested socket option -- confirmed empirically that the sockopt is correctly False immediately after setsockopt(), and only gets reset by the kernel at bind() time, specifically for '::1' and not for the wildcard '::'. This is sensible kernel behavior (a loopback-only bind can never be meaningfully dual-stack, since there's no IPv4-loopback equivalent of '::1'), not a bug -- but it meant the parametrized test could never observe ipv6_v6only=False actually taking effect. Switch the test to bind '::' instead, which is also the actual real-world case the whole PR is about. Verified passing on real Linux via Docker (python:3.11-slim), all 6 IPv6-related tests across test_config.py, test_server.py, and test_main.py green.
|
Pushed fixes for everything flagged by review: Fixed:
CI failure — root cause found and fixed: Setting All 993 local tests still pass, |
The new `except SystemExit: await self.lifespan.shutdown(); raise` branch in the explicit-bind path had no test exercising it, dropping this project's required 100% coverage to 99.96% (server.py 191/194 -> 98.43%). Add test_ipv6_v6only_bind_failure_runs_lifespan_shutdown: occupies an IPv6 port, configures the server to explicit-bind that same port (via ipv6_v6only=<any value>), and asserts both that SystemExit propagates and that lifespan.shutdown() actually ran. Verified 100% coverage on server.py locally via Docker (python:3.11-slim) with the project's own coverage config (COVERAGE_PROCESS_START + parallel combine, matching scripts/coverage).
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Looked into the latest CI run — every platform/version job is failing on exactly one different, unrelated test each time (e.g. `test_run_chain_only` on 3.12-ubuntu, `test_limit_max_requests_jitter[zttp]` on 3.12-macos, `test_request_than_limit_max_requests_warn_log[h11]` on 3.11-ubuntu, etc.), with the actual functional test counts all passing (1000–1270 tests green in each job). Windows jobs passed cleanly with 0 errors. That pattern — one random unrelated test erroring per run, varying every time — looks like CI resource contention under full parallel load rather than anything in this PR; I don't see how the changes here would cause an unrelated test like I don't have permission to re-run the workflow as an outside contributor. Happy to take another look if a maintainer re-triggers it and something reproducible shows up. |
bind_socket() created a socket, and on a bind OSError logged + called sys.exit() without closing it first, leaking the fd. The new IPv6 bind-failure test was the first test to actually exercise this path, so the leaked socket surfaced as a ResourceWarning at teardown (pytest turns these into a hard failure), which is why every job in the last CI run failed identically across all platforms including Windows. Also move the test's own blocker-socket setup inside try/finally, per cubic review feedback, so a bind/listen failure on the test's side can't leak a socket either.
|
Found the real root cause of the CI failures — retracting my earlier "unrelated flakiness" theory, it wasn't that.
Fixed in
Verified locally with |
The test's win32 skip was copied from a sibling test that genuinely depends on '::' dual-stack accept semantics differing on Windows. This test doesn't: it only needs bind() to raise on an already-bound port, which is standard OS behavior everywhere, and Python normalizes socket errors to OSError cross-platform. Skipping it left the new `except SystemExit` cleanup path in server.py uncovered specifically on Windows CI, failing the 100%-coverage gate on every Windows job.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…m its coverage gate instead Un-skipping it last commit was wrong: cubic's review correctly called out that Windows' looser SO_REUSEADDR semantics let both sockets bind the same wildcard address, so the bind conflict this test relies on doesn't reliably happen there. Confirmed directly -- the Windows CI jobs hung for ~8.5 minutes and were cancelled rather than failing cleanly, exactly matching that prediction (pytest.raises(SystemExit) never firing because bind() never raised). Restored the win32 skip, and instead excluded the corresponding `except SystemExit` branch in server.py from Windows' 100%-coverage requirement via `# pragma: py-win32`, matching how this codebase already excludes other platform-untestable branches (e.g. the UDS path, which doesn't exist on Windows either).
|
Good catch, and confirmed the hard way: the Windows CI jobs on the un-skip commit hung for ~8.5 minutes and got cancelled rather than failing cleanly — exactly matching your prediction that `SO_REUSEADDR` lets both sockets bind on Windows, so `bind()` never raises and `pytest.raises(SystemExit)` never sees anything. Reverted in `7784f59`: restored the win32 skip on `test_ipv6_v6only_bind_failure_runs_lifespan_shutdown`, and instead excluded the corresponding `except SystemExit` branch in `server.py` from Windows' 100%-coverage requirement via `# pragma: py-win32` (same pattern this codebase already uses for other platform-untestable branches, e.g. the UDS path). Verified locally and confirmed Linux/macOS still cover that branch via the non-Windows run of the same test. |
…erage gate The last commit's pragma on `except SystemExit` wasn't enough: the Windows coverage failure it left behind (182-183, now 173-186) was actually the *whole* explicit-bind `if` branch in startup() -- reachable only via the two ipv6_v6only tests, both skipped on Windows for the same underlying reason (SO_REUSEADDR/dual-stack quirks). Since neither test runs there, the branch itself was never exercised on that platform. Excluded it with `# pragma: py-win32` on the `if`, mirroring how this file already excludes the equally Windows-untestable UDS branch (`elif config.uds is not None: # pragma: py-win32`). Also added the same pragma to the two win32-skipped test functions themselves (matching test_config.py's existing convention for win32-skipped tests), since their bodies were showing up as additional missing lines in tests/test_server.py.
1 participant
Summary
Fixes #2945.
Binding an IPv6 host currently behaves inconsistently depending on worker count:
Server.startup()delegates toloop.create_server(host=..., port=...), and asyncio unconditionally setsIPV6_V6ONLY=Truefor IPv6 sockets.Config.bind_socket()creates the socket manually and never touchesIPV6_V6ONLY, so it inherits the OS default (dual-stack on Linux).So
uvicorn app:app --host '::'silently drops IPv4 connectivity by default, but accepts it with--workers 2+— as reported, this is a real problem for e.g. rootless Podman/pasta networking where IPv4 forwarding has nowhere to connect if the server is IPv6-only.Approach
I deliberately did not change either existing default (that's a judgment call — dual-stack-by-default vs IPv6-only-by-default has security/deployment implications either way and isn't mine to make). Instead this adds an explicit, opt-in
ipv6_v6only: bool | None = NoneConfigoption (+--ipv6-v6only/--no-ipv6-v6onlyCLI flag) that's a no-op when left unset:bind_socket()now setsIPV6_V6ONLYexplicitly wheneveripv6_v6only is not None— this already covers the multi-worker path, which callsbind_socket()directly.Server.startup()now also routes throughbind_socket()(passing the socket viasock=instead ofhost=/port=) wheneveripv6_v6onlyis explicitly set, sinceasyncio.create_server(host=, port=)would otherwise silently override any socket option set beforehand.bind_socket()gained alog: bool = Trueparameter so the single-worker path can passlog=Falseand keep using the existing_log_started_message()for its startup banner, avoiding a duplicate log line.With
ipv6_v6onlyleft at its defaultNone, neither code path's behavior changes at all — purely additive.Test plan
--host '::'accepts IPv6 but refuses IPv4 by default;ipv6_v6only=Falsefixes it end-to-end (verified with a realasyncio.open_connectionover both127.0.0.1and::1).IPV6_V6ONLYstill ends upTruefor single-worker with no explicit setting).bind_socket(log=False).test_bind_socket_ipv6_v6only_*intests/test_config.py(explicit True/False, unset no-op, ignored for IPv4 hosts).test_ipv6_v6only_false_accepts_ipv4_in_single_worker_modeintests/test_server.py— real end-to-end server test over httpx, single-worker mode, both IPv4 and IPv6 clients.pytest tests/ --ignore=tests/benchmarks→ 993 passed (up from 988 pre-change), 252 skipped.ruff format --check,ruff check, andmypy uvicorn testsall clean.docs/settings.mdunder Socket Binding.Summary by CodeRabbit
New Features
--ipv6-v6onlyand--no-ipv6-v6onlyoptions to control IPv6 socket behavior.Bug Fixes
Documentation