diff --git a/openhands/app_server/sandbox/remote_sandbox_service.py b/openhands/app_server/sandbox/remote_sandbox_service.py index 2a95a3776c..6287f99d61 100644 --- a/openhands/app_server/sandbox/remote_sandbox_service.py +++ b/openhands/app_server/sandbox/remote_sandbox_service.py @@ -53,14 +53,6 @@ from openhands.sdk.utils.paging import page_iterator _logger = logging.getLogger(__name__) polling_task: asyncio.Task | None = None -POD_STATUS_MAPPING = { - 'ready': SandboxStatus.RUNNING, - 'pending': SandboxStatus.STARTING, - 'running': SandboxStatus.STARTING, - 'failed': SandboxStatus.ERROR, - 'unknown': SandboxStatus.ERROR, - 'crashloopbackoff': SandboxStatus.ERROR, -} STATUS_MAPPING = { 'running': SandboxStatus.RUNNING, 'paused': SandboxStatus.PAUSED, @@ -188,28 +180,22 @@ class RemoteSandboxService(SandboxService): def _get_sandbox_status_from_runtime( self, runtime: dict[str, Any] | None ) -> SandboxStatus: - """Derive a SandboxStatus from the runtime info. The legacy logic for getting - the status of a runtime is inconsistent. It is divided between a "status" which - cannot be trusted (It sometimes returns "running" for cases when the pod is - still starting) and a "pod_status" which is not returned for list - operations.""" + """Derive a SandboxStatus from the runtime info. + + The status field is now the source of truth for sandbox status. It accounts + for both pod readiness and ingress availability, making it more reliable than + pod_status which only reflected pod state. + """ if not runtime: return SandboxStatus.MISSING - status = None - pod_status = (runtime.get('pod_status') or '').lower() - if pod_status: - status = POD_STATUS_MAPPING.get(pod_status, None) + runtime_status = runtime.get('status') + if runtime_status: + status = STATUS_MAPPING.get(runtime_status.lower(), None) + if status is not None: + return status - # If we failed to get the status from the pod status, fall back to status - if status is None: - runtime_status = runtime.get('status') - if runtime_status: - status = STATUS_MAPPING.get(runtime_status.lower(), None) - - if status is None: - return SandboxStatus.MISSING - return status + return SandboxStatus.MISSING async def _secure_select(self): query = select(StoredRemoteSandbox) @@ -514,9 +500,6 @@ class RemoteSandboxService(SandboxService): session_api_key ) - # Hack - result doesn't contain this - runtime_data['pod_status'] = 'pending' - # Log runtime assignment for observability runtime_id = runtime_data.get('runtime_id', 'unknown') _logger.info(f'Started sandbox {sandbox_id} with runtime_id={runtime_id}') diff --git a/tests/unit/app_server/test_remote_sandbox_service.py b/tests/unit/app_server/test_remote_sandbox_service.py index 7e8a2178dd..a7547092ef 100644 --- a/tests/unit/app_server/test_remote_sandbox_service.py +++ b/tests/unit/app_server/test_remote_sandbox_service.py @@ -22,7 +22,6 @@ from sqlalchemy.ext.asyncio import AsyncSession from openhands.app_server.errors import SandboxError from openhands.app_server.sandbox.remote_sandbox_service import ( ALLOW_CORS_ORIGINS_VARIABLE, - POD_STATUS_MAPPING, STATUS_MAPPING, WEBHOOK_CALLBACK_VARIABLE, RemoteSandboxService, @@ -98,7 +97,6 @@ def remote_sandbox_service( def create_runtime_data( session_id: str = 'test-sandbox-123', status: str = 'running', - pod_status: str = 'ready', url: str = 'https://sandbox.example.com', session_api_key: str = 'test-session-key', runtime_id: str = 'runtime-456', @@ -107,7 +105,6 @@ def create_runtime_data( return { 'session_id': session_id, 'status': status, - 'pod_status': pod_status, 'url': url, 'session_api_key': session_api_key, 'runtime_id': runtime_id, @@ -188,24 +185,11 @@ class TestStatusMapping: """Test cases for status mapping functionality.""" @pytest.mark.asyncio - async def test_get_sandbox_status_from_runtime_with_pod_status( + async def test_get_sandbox_status_from_runtime_with_status( self, remote_sandbox_service ): - """Test status mapping using pod_status.""" - runtime_data = create_runtime_data(pod_status='ready') - - status = remote_sandbox_service._get_sandbox_status_from_runtime(runtime_data) - - assert status == SandboxStatus.RUNNING - - @pytest.mark.asyncio - async def test_get_sandbox_status_from_runtime_fallback_to_status( - self, remote_sandbox_service - ): - """Test status mapping fallback to status field.""" - runtime_data = create_runtime_data( - pod_status='unknown_pod_status', status='running' - ) + """Test status mapping using status field.""" + runtime_data = create_runtime_data(status='running') status = remote_sandbox_service._get_sandbox_status_from_runtime(runtime_data) @@ -225,32 +209,22 @@ class TestStatusMapping: self, remote_sandbox_service ): """Test status mapping with unknown status values.""" - runtime_data = create_runtime_data( - pod_status='unknown_pod', status='unknown_status' - ) + runtime_data = create_runtime_data(status='unknown_status') status = remote_sandbox_service._get_sandbox_status_from_runtime(runtime_data) assert status == SandboxStatus.MISSING @pytest.mark.asyncio - async def test_pod_status_mapping_coverage(self, remote_sandbox_service): - """Test all pod status mappings are handled correctly.""" - test_cases = [ - ('ready', SandboxStatus.RUNNING), - ('pending', SandboxStatus.STARTING), - ('running', SandboxStatus.STARTING), - ('failed', SandboxStatus.ERROR), - ('unknown', SandboxStatus.ERROR), - ('crashloopbackoff', SandboxStatus.ERROR), - ] + async def test_get_sandbox_status_from_runtime_empty_status( + self, remote_sandbox_service + ): + """Test status mapping with empty status field.""" + runtime_data = create_runtime_data(status='') - for pod_status, expected_status in test_cases: - runtime_data = create_runtime_data(pod_status=pod_status) - status = remote_sandbox_service._get_sandbox_status_from_runtime( - runtime_data - ) - assert status == expected_status, f'Failed for pod_status: {pod_status}' + status = remote_sandbox_service._get_sandbox_status_from_runtime(runtime_data) + + assert status == SandboxStatus.MISSING @pytest.mark.asyncio async def test_status_mapping_coverage(self, remote_sandbox_service): @@ -264,8 +238,24 @@ class TestStatusMapping: ] for status, expected_status in test_cases: - # Use empty pod_status to force fallback to status field - runtime_data = create_runtime_data(pod_status='', status=status) + runtime_data = create_runtime_data(status=status) + result = remote_sandbox_service._get_sandbox_status_from_runtime( + runtime_data + ) + assert result == expected_status, f'Failed for status: {status}' + + @pytest.mark.asyncio + async def test_status_mapping_case_insensitive(self, remote_sandbox_service): + """Test that status mapping is case-insensitive.""" + test_cases = [ + ('RUNNING', SandboxStatus.RUNNING), + ('Running', SandboxStatus.RUNNING), + ('PAUSED', SandboxStatus.PAUSED), + ('Starting', SandboxStatus.STARTING), + ] + + for status, expected_status in test_cases: + runtime_data = create_runtime_data(status=status) result = remote_sandbox_service._get_sandbox_status_from_runtime( runtime_data ) @@ -336,7 +326,7 @@ class TestSandboxInfoConversion: """Test conversion to SandboxInfo with running runtime.""" # Setup stored_sandbox = create_stored_sandbox() - runtime_data = create_runtime_data(status='running', pod_status='ready') + runtime_data = create_runtime_data(status='running') # Execute sandbox_info = remote_sandbox_service._to_sandbox_info( @@ -363,7 +353,7 @@ class TestSandboxInfoConversion: """Test conversion to SandboxInfo with starting runtime.""" # Setup stored_sandbox = create_stored_sandbox() - runtime_data = create_runtime_data(status='running', pod_status='pending') + runtime_data = create_runtime_data(status='starting') # Execute sandbox_info = remote_sandbox_service._to_sandbox_info( @@ -400,7 +390,7 @@ class TestSandboxLifecycle: """Test successful sandbox start.""" # Setup mock_response = MagicMock() - mock_response.json.return_value = create_runtime_data() + mock_response.json.return_value = create_runtime_data(status='running') remote_sandbox_service.httpx_client.request.return_value = mock_response remote_sandbox_service.pause_old_sandboxes = AsyncMock(return_value=[]) @@ -414,9 +404,7 @@ class TestSandboxLifecycle: # Verify assert sandbox_info.id == 'test-sandbox-123' - assert ( - sandbox_info.status == SandboxStatus.STARTING - ) # pod_status is 'pending' by default + assert sandbox_info.status == SandboxStatus.RUNNING remote_sandbox_service.pause_old_sandboxes.assert_called_once_with( 9 ) # max_num_sandboxes - 1 @@ -1267,19 +1255,6 @@ class TestUtilityFunctions: class TestConstants: """Test cases for constants and mappings.""" - def test_pod_status_mapping_completeness(self): - """Test that POD_STATUS_MAPPING covers expected statuses.""" - expected_statuses = [ - 'ready', - 'pending', - 'running', - 'failed', - 'unknown', - 'crashloopbackoff', - ] - for status in expected_statuses: - assert status in POD_STATUS_MAPPING, f'Missing pod status: {status}' - def test_status_mapping_completeness(self): """Test that STATUS_MAPPING covers expected statuses.""" expected_statuses = ['running', 'paused', 'stopped', 'starting', 'error']