fix(vllm): support IPv6 communicator hosts - #6907
Conversation
|
Ready for review after contributor approval. The PR follows the focused communicator-boundary scope discussed in #3164; template and AI disclosure are complete. Thank you for taking a look. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Addressed the Bugbot finding in the latest commit: communicator hosts now strip URL-only IPv6 brackets, preserve IP literals, and retain the previous socket.gethostbyname behavior for hostnames such as localhost. Added a mocked hostname-resolution regression alongside the IPv6 cases. Local branch-source helper validation passes for IPv4, hostname, and bracketed/bare IPv6. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ca910ca. Configure here.
| self.base_url = f"{scheme}://{parsed_url.netloc}{parsed_url.path}" | ||
| else: | ||
| self.host = host | ||
| self.host = _resolve_communicator_host(host) |
There was a problem hiding this comment.
Host path rewrites HTTP to IPv4
Medium Severity
_resolve_communicator_host now runs socket.gethostbyname on the host argument, then _format_http_host builds base_url from that already-resolved value. A hostname such as localhost becomes an IPv4 HTTP URL and communicator address, so IPv6-only or IPv6-preferred hosts fail even though the base_url path still keeps the original URL separate from the communicator host.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ca910ca. Configure here.
| if response.status_code == 200: | ||
| if "X-Forwarded-For" in response.headers: | ||
| self.host = response.headers["X-Forwarded-For"] | ||
| self.host = _resolve_communicator_host( |
There was a problem hiding this comment.
Forwarded host resolution can crash
Medium Severity
A successful health check now passes X-Forwarded-For through _resolve_communicator_host. Multi-hop or non-literal values are not valid IPs, so socket.gethostbyname raises socket.gaierror. That exception is outside the RequestException handler, so a 200 health response can crash client setup.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ca910ca. Configure here.
1 participant
What does this PR do?
Fixes #3164.
This fixes IPv6 weight-sync initialization in
VLLMClient:http://[2001:db8::1]:8000.2001:db8::1.The previous
socket.gethostbyname()conversion is IPv4-only and cannot represent an IPv6 literal. The change keeps the HTTP and communicator address contracts separate and normalizes forwarded host headers as well.Before submitting
AI writing disclosure
Validation
Who can review?
@qgallouedec, since you requested this focused communicator-boundary fix in #3164.
Note
Medium Risk
Touches distributed weight-sync addressing (TCPStore/NCCL); behavior change is scoped but failures would break training sync on IPv6 or proxied setups.
Overview
Fixes IPv6 weight-sync setup in
VLLMClientby separating how hosts are formatted for HTTP vs for NCCL/TCPStore.VLLMClientno longer always runs the server host throughsocket.gethostbyname()(IPv4-only). New helpers normalize addresses:_resolve_communicator_hostkeeps bare IPv6 literals (and bracket-stripped forms) for the communicator, still resolving hostnames viagethostbyname;_format_http_hostadds brackets when buildinghttp://…URLs. The same resolution applies whenX-Forwarded-Forupdatesself.host.Regression tests in
TestVLLMClientAddressingcover bracketed/bare IPv6, IPv4, and hostname resolution.Reviewed by Cursor Bugbot for commit ca910ca. Bugbot is set up for automated code reviews on this repo. Configure here.