Fix for docker regression (#11759)

tests/unit/app_server/test_docker_sandbox_service.py

@@ -697,10 +697,14 @@ class TestDockerSandboxService:
         assert result is not None
         assert isinstance(result.created_at, datetime)
 
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
     async def test_container_to_checked_sandbox_info_health_check_success(
-        self, service, mock_running_container
+        self, mock_is_docker, service, mock_running_container
     ):
-        """Test health check success."""
+        """Test health check success when running in Docker."""
         # Setup
         service.httpx_client.get.return_value.raise_for_status.return_value = None
 
@@ -715,7 +719,34 @@ class TestDockerSandboxService:
         assert result.exposed_urls is not None
         assert result.session_api_key == 'session_key_123'
 
-        # Verify health check was called
+        # Verify health check was called with Docker-internal URL
+        service.httpx_client.get.assert_called_once_with(
+            'http://host.docker.internal:12345/health'
+        )
+
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=False,
+    )
+    async def test_container_to_checked_sandbox_info_health_check_success_not_in_docker(
+        self, mock_is_docker, service, mock_running_container
+    ):
+        """Test health check success when not running in Docker."""
+        # Setup
+        service.httpx_client.get.return_value.raise_for_status.return_value = None
+
+        # Execute
+        result = await service._container_to_checked_sandbox_info(
+            mock_running_container
+        )
+
+        # Verify
+        assert result is not None
+        assert result.status == SandboxStatus.RUNNING
+        assert result.exposed_urls is not None
+        assert result.session_api_key == 'session_key_123'
+
+        # Verify health check was called with original localhost URL
         service.httpx_client.get.assert_called_once_with(
             'http://localhost:12345/health'
         )

tests/unit/app_server/test_docker_utils.py

@@ -1,31 +1,63 @@
-from openhands.app_server.utils.docker_utils import replace_localhost_hostname
+from unittest.mock import patch
+
+from openhands.app_server.utils.docker_utils import (
+    replace_localhost_hostname_for_docker,
+)
 
 
-class TestReplaceLocalhostHostname:
-    """Test cases for replace_localhost_hostname function."""
+class TestReplaceLocalhostHostnameForDocker:
+    """Test cases for replace_localhost_hostname_for_docker function."""
 
-    def test_replace_localhost_basic(self):
-        """Test basic localhost replacement."""
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_replace_localhost_basic_in_docker(self, mock_is_docker):
+        """Test basic localhost replacement when running in Docker."""
         # Basic HTTP URL
-        result = replace_localhost_hostname('http://localhost:8080')
+        result = replace_localhost_hostname_for_docker('http://localhost:8080')
         assert result == 'http://host.docker.internal:8080'
 
         # HTTPS URL
-        result = replace_localhost_hostname('https://localhost:443')
+        result = replace_localhost_hostname_for_docker('https://localhost:443')
         assert result == 'https://host.docker.internal:443'
 
         # No port specified
-        result = replace_localhost_hostname('http://localhost')
+        result = replace_localhost_hostname_for_docker('http://localhost')
         assert result == 'http://host.docker.internal'
 
-    def test_replace_localhost_with_path_and_query(self):
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=False,
+    )
+    def test_replace_localhost_basic_not_in_docker(self, mock_is_docker):
+        """Test that localhost is NOT replaced when not running in Docker."""
+        # Basic HTTP URL
+        result = replace_localhost_hostname_for_docker('http://localhost:8080')
+        assert result == 'http://localhost:8080'
+
+        # HTTPS URL
+        result = replace_localhost_hostname_for_docker('https://localhost:443')
+        assert result == 'https://localhost:443'
+
+        # No port specified
+        result = replace_localhost_hostname_for_docker('http://localhost')
+        assert result == 'http://localhost'
+
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_replace_localhost_with_path_and_query(self, mock_is_docker):
         """Test localhost replacement preserving path and query parameters."""
         # With path
-        result = replace_localhost_hostname('http://localhost:3000/api/health')
+        result = replace_localhost_hostname_for_docker(
+            'http://localhost:3000/api/health'
+        )
         assert result == 'http://host.docker.internal:3000/api/health'
 
         # With query parameters containing localhost
-        result = replace_localhost_hostname(
+        result = replace_localhost_hostname_for_docker(
             'http://localhost:8080/path?param=localhost&other=value'
         )
         assert (
@@ -34,94 +66,158 @@ class TestReplaceLocalhostHostname:
         )
 
         # With path containing localhost
-        result = replace_localhost_hostname('http://localhost:9000/localhost/endpoint')
+        result = replace_localhost_hostname_for_docker(
+            'http://localhost:9000/localhost/endpoint'
+        )
         assert result == 'http://host.docker.internal:9000/localhost/endpoint'
 
         # With fragment
-        result = replace_localhost_hostname('http://localhost:8080/path#localhost')
+        result = replace_localhost_hostname_for_docker(
+            'http://localhost:8080/path#localhost'
+        )
         assert result == 'http://host.docker.internal:8080/path#localhost'
 
-    def test_replace_localhost_with_authentication(self):
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_replace_localhost_with_authentication(self, mock_is_docker):
         """Test localhost replacement with authentication in URL."""
-        result = replace_localhost_hostname('http://user:pass@localhost:8080/path')
+        result = replace_localhost_hostname_for_docker(
+            'http://user:pass@localhost:8080/path'
+        )
         assert result == 'http://user:pass@host.docker.internal:8080/path'
 
-        result = replace_localhost_hostname('https://admin:secret@localhost:443/admin')
+        result = replace_localhost_hostname_for_docker(
+            'https://admin:secret@localhost:443/admin'
+        )
         assert result == 'https://admin:secret@host.docker.internal:443/admin'
 
-    def test_replace_localhost_different_protocols(self):
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_replace_localhost_different_protocols(self, mock_is_docker):
         """Test localhost replacement with different protocols."""
         # FTP
-        result = replace_localhost_hostname('ftp://localhost:21/files')
+        result = replace_localhost_hostname_for_docker('ftp://localhost:21/files')
         assert result == 'ftp://host.docker.internal:21/files'
 
         # WebSocket
-        result = replace_localhost_hostname('ws://localhost:8080/socket')
+        result = replace_localhost_hostname_for_docker('ws://localhost:8080/socket')
         assert result == 'ws://host.docker.internal:8080/socket'
 
         # WebSocket Secure
-        result = replace_localhost_hostname('wss://localhost:443/secure-socket')
+        result = replace_localhost_hostname_for_docker(
+            'wss://localhost:443/secure-socket'
+        )
         assert result == 'wss://host.docker.internal:443/secure-socket'
 
-    def test_no_replacement_for_non_localhost(self):
-        """Test that non-localhost hostnames are not replaced."""
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_no_replacement_for_non_localhost(self, mock_is_docker):
+        """Test that non-localhost hostnames are not replaced even when in Docker."""
         # IP address
-        result = replace_localhost_hostname('http://127.0.0.1:8080')
+        result = replace_localhost_hostname_for_docker('http://127.0.0.1:8080')
         assert result == 'http://127.0.0.1:8080'
 
         # Different hostname
-        result = replace_localhost_hostname('http://example.com:8080')
+        result = replace_localhost_hostname_for_docker('http://example.com:8080')
         assert result == 'http://example.com:8080'
 
         # Hostname containing localhost but not exact match
-        result = replace_localhost_hostname('http://mylocalhost:8080')
+        result = replace_localhost_hostname_for_docker('http://mylocalhost:8080')
         assert result == 'http://mylocalhost:8080'
 
         # Subdomain of localhost
-        result = replace_localhost_hostname('http://api.localhost:8080')
+        result = replace_localhost_hostname_for_docker('http://api.localhost:8080')
         assert result == 'http://api.localhost:8080'
 
         # localhost as subdomain
-        result = replace_localhost_hostname('http://localhost.example.com:8080')
+        result = replace_localhost_hostname_for_docker(
+            'http://localhost.example.com:8080'
+        )
         assert result == 'http://localhost.example.com:8080'
 
-    def test_custom_replacement_hostname(self):
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_custom_replacement_hostname(self, mock_is_docker):
         """Test using custom replacement hostname."""
-        result = replace_localhost_hostname('http://localhost:8080', 'custom.host')
+        result = replace_localhost_hostname_for_docker(
+            'http://localhost:8080', 'custom.host'
+        )
         assert result == 'http://custom.host:8080'
 
-        result = replace_localhost_hostname(
+        result = replace_localhost_hostname_for_docker(
             'https://localhost:443/path', 'internal.docker'
         )
         assert result == 'https://internal.docker:443/path'
 
-    def test_edge_cases(self):
-        """Test edge cases and malformed URLs."""
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_edge_cases_in_docker(self, mock_is_docker):
+        """Test edge cases and malformed URLs when in Docker."""
         # Empty string
-        result = replace_localhost_hostname('')
+        result = replace_localhost_hostname_for_docker('')
         assert result == ''
 
         # Malformed URL (no protocol)
-        result = replace_localhost_hostname('localhost:8080')
+        result = replace_localhost_hostname_for_docker('localhost:8080')
         assert result == 'localhost:8080'
 
         # Just hostname
-        result = replace_localhost_hostname('localhost')
+        result = replace_localhost_hostname_for_docker('localhost')
         assert result == 'localhost'
 
         # URL with no hostname
-        result = replace_localhost_hostname('http://:8080/path')
+        result = replace_localhost_hostname_for_docker('http://:8080/path')
         assert result == 'http://:8080/path'
 
         # Invalid URL structure
-        result = replace_localhost_hostname('not-a-url')
+        result = replace_localhost_hostname_for_docker('not-a-url')
         assert result == 'not-a-url'
 
-    def test_complex_urls(self):
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=False,
+    )
+    def test_edge_cases_not_in_docker(self, mock_is_docker):
+        """Test edge cases and malformed URLs when not in Docker."""
+        # Empty string
+        result = replace_localhost_hostname_for_docker('')
+        assert result == ''
+
+        # Malformed URL (no protocol)
+        result = replace_localhost_hostname_for_docker('localhost:8080')
+        assert result == 'localhost:8080'
+
+        # Just hostname
+        result = replace_localhost_hostname_for_docker('localhost')
+        assert result == 'localhost'
+
+        # URL with no hostname
+        result = replace_localhost_hostname_for_docker('http://:8080/path')
+        assert result == 'http://:8080/path'
+
+        # Invalid URL structure
+        result = replace_localhost_hostname_for_docker('not-a-url')
+        assert result == 'not-a-url'
+
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_complex_urls(self, mock_is_docker):
         """Test complex URL scenarios."""
         # Multiple query parameters and fragments
         complex_url = 'http://localhost:8080/api/v1/health?timeout=30&retry=3&host=localhost#section'
-        result = replace_localhost_hostname(complex_url)
+        result = replace_localhost_hostname_for_docker(complex_url)
         expected = 'http://host.docker.internal:8080/api/v1/health?timeout=30&retry=3&host=localhost#section'
         assert result == expected
 
@@ -129,17 +225,21 @@ class TestReplaceLocalhostHostname:
         encoded_url = (
             'http://localhost:8080/path%20with%20spaces?param=value%20with%20spaces'
         )
-        result = replace_localhost_hostname(encoded_url)
+        result = replace_localhost_hostname_for_docker(encoded_url)
         expected = 'http://host.docker.internal:8080/path%20with%20spaces?param=value%20with%20spaces'
         assert result == expected
 
-    def test_integration_with_docker_detection(self):
-        """Test integration scenario similar to actual usage."""
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_integration_with_docker_detection_in_docker(self, mock_is_docker):
+        """Test integration scenario similar to actual usage when in Docker."""
         # Simulate the actual usage pattern in the code
         app_server_url = 'http://localhost:35375'
 
         # This is how it's used in the actual code
-        internal_url = replace_localhost_hostname(app_server_url)
+        internal_url = replace_localhost_hostname_for_docker(app_server_url)
 
         assert internal_url == 'http://host.docker.internal:35375'
 
@@ -147,10 +247,33 @@ class TestReplaceLocalhostHostname:
         health_check_url = f'{internal_url}/health'
         assert health_check_url == 'http://host.docker.internal:35375/health'
 
-    def test_preserves_original_url_structure(self):
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=False,
+    )
+    def test_integration_with_docker_detection_not_in_docker(self, mock_is_docker):
+        """Test integration scenario similar to actual usage when not in Docker."""
+        # Simulate the actual usage pattern in the code
+        app_server_url = 'http://localhost:35375'
+
+        # This is how it's used in the actual code
+        internal_url = replace_localhost_hostname_for_docker(app_server_url)
+
+        # Should return original URL when not in Docker
+        assert internal_url == 'http://localhost:35375'
+
+        # Test with health check path appended
+        health_check_url = f'{internal_url}/health'
+        assert health_check_url == 'http://localhost:35375/health'
+
+    @patch(
+        'openhands.app_server.utils.docker_utils.is_running_in_docker',
+        return_value=True,
+    )
+    def test_preserves_original_url_structure(self, mock_is_docker):
         """Test that all URL components are preserved correctly."""
         original_url = 'https://user:pass@localhost:8443/api/v1/endpoint?param1=value1&param2=value2#fragment'
-        result = replace_localhost_hostname(original_url)
+        result = replace_localhost_hostname_for_docker(original_url)
         expected = 'https://user:pass@host.docker.internal:8443/api/v1/endpoint?param1=value1&param2=value2#fragment'
 
         assert result == expected