Skip to content

fix(ide): add async DNS check for host.docker.internal in container environments#1817

Open
yiliang114 wants to merge 1 commit intoQwenLM:mainfrom
yiliang114:fix/docker-ide-host-check
Open

fix(ide): add async DNS check for host.docker.internal in container environments#1817
yiliang114 wants to merge 1 commit intoQwenLM:mainfrom
yiliang114:fix/docker-ide-host-check

Conversation

@yiliang114
Copy link
Collaborator

TLDR

Fix IDE client connection failure in container environments (e.g. code-server / WebIDE) where host.docker.internal is not available. The getIdeServerHost() function is now async and performs a DNS lookup to verify host.docker.internal is resolvable before using it, automatically falling back to 127.0.0.1 when it is not.

Dive Deeper

Problem

In Docker Desktop, host.docker.internal is automatically configured to resolve to the host machine. However, this hostname may not exist in:

  • Linux Docker without --add-host=host.docker.internal:host-gateway
  • code-server / WebIDE environments that run inside containers but not via Docker Desktop
  • Other container runtimes like Podman that may not support this hostname

The previous implementation detected container environments via /.dockerenv or /run/.containerenv and unconditionally used host.docker.internal, causing IDE connection failures in environments where that hostname is unresolvable.

Changes

  1. Async getIdeServerHost(): Now performs a dns.lookup to check if host.docker.internal is resolvable before using it
  2. Result caching: First lookup result is cached in a module-level variable to avoid repeated DNS queries
  3. Debug logging: Logs DNS check results in container environments to aid troubleshooting
  4. Test helper: Exported _resetCachedIdeServerHost() for test isolation
  5. Comprehensive tests: Added 6 new test cases covering all scenarios (non-container, container with reachable host, container with unreachable host, Podman container detection, caching behavior, and end-to-end HTTP connection URL verification)

Reviewer Test Plan

  1. Pull the branch and run tests: npx vitest run packages/core/src/ide/ide-client.test.ts
  2. Verify all 24 tests pass (18 existing + 6 new)
  3. (Optional) In a Docker container with host.docker.internal configured, verify IDE connects normally
  4. (Optional) In a code-server environment without host.docker.internal, verify IDE falls back to 127.0.0.1

Testing Matrix

🍏 🪟 🐧
npm run
npx
Docker
Podman - -
Seatbelt - -

Linked issues / bugs

Fixes #1567

…nvironments

In container environments like code-server where host.docker.internal may
not be configured, the IDE client would fail to connect. This change makes
getIdeServerHost() async and adds a DNS lookup to verify the hostname is
actually resolvable before using it, falling back to 127.0.0.1 if not.

Co-authored-by: Cursor <[email protected]>
@github-actions
Copy link
Contributor

📋 Review Summary

This PR addresses an important issue where the IDE client connection fails in container environments where host.docker.internal is not available. The solution involves making getIdeServerHost() asynchronous and performing a DNS lookup to verify if host.docker.internal is resolvable before using it, with fallback to 127.0.0.1 when it's not reachable. The implementation includes caching to avoid repeated DNS queries and comprehensive test coverage.

🔍 General Feedback

  • The implementation correctly addresses the problem of IDE connection failures in container environments without host.docker.internal
  • Good use of caching to prevent repeated DNS lookups and improve performance
  • Comprehensive test coverage added for all scenarios (non-container, container with reachable host, container with unreachable host, etc.)
  • Clear documentation and comments explaining the rationale behind the changes
  • Proper separation of concerns with the checkHostReachable helper function

🎯 Specific Feedback

🟡 High

  • File: packages/core/src/ide/ide-client.ts:855 - The caching mechanism uses a module-level variable cachedIdeServerHost which could lead to unexpected behavior in certain edge cases. Consider if the cache should be invalidated under specific conditions (e.g., network changes) or if a more sophisticated caching strategy is needed.

🟢 Medium

  • File: packages/core/src/ide/ide-client.ts:809 - The call to getIdeServerHost() in establishHttpConnection() is now awaited, which is correct, but this means the connection establishment is now slightly delayed by the DNS lookup. This is acceptable given the reliability improvement, but worth noting for performance considerations.
  • File: packages/core/src/ide/ide-client.ts:876 - The checkHostReachable function uses a Promise wrapper around the callback-based dns.lookup. This is fine, but consider using the promisified version dns.promises.lookup for cleaner code.

🔵 Low

  • File: packages/core/src/ide/ide-client.ts:888 - The JSDoc comment mentions "code-server remote" as an example of an environment where host.docker.internal may not exist. Consider adding a reference to the issue number (cli cannot link to vscode under code-server(webide) architecture #1567) in the comment for better traceability.
  • File: packages/core/src/ide/ide-client.test.ts:53 - The comment mentions resetting both singleton instance and cached host for test isolation, which is good practice and properly implemented.

✅ Highlights

  • Excellent test coverage with 6 new test cases covering all scenarios (non-container, container with reachable host, unreachable host, Podman detection, caching behavior, and end-to-end HTTP connection)
  • Smart caching implementation to avoid repeated DNS lookups while maintaining correctness
  • Proper error handling with fallback to 127.0.0.1 when host.docker.internal is not reachable
  • Exported _resetCachedIdeServerHost() function for proper test isolation
  • Good debug logging to help troubleshoot connection issues in container environments

*/
function checkHostReachable(hostname: string): Promise<boolean> {
return new Promise((resolve) => {
dns.lookup(hostname, (err) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dns.lookup has no timeout — under poor network conditions it could block for tens of seconds. Consider wrapping it with a ~3s setTimeout fallback to keep the connection flow snappy.

Rest looks great, solid test coverage 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cli cannot link to vscode under code-server(webide) architecture

2 participants

Comments