From 906aab3b73fa461b9790ffdcda26ec9b6c8cd280 Mon Sep 17 00:00:00 2001 From: Alex Blank Date: Tue, 2 Jun 2026 13:41:07 +0200 Subject: [PATCH] fix: workspace creation with stale directories and missing bind mount - workspace_manager.py: remove stale workspace directories before cloning to prevent 'already exists' errors from previous failed attempts - workspaces.py: add ValueError -> 400 handling, keep 409 for duplicates - test_tool_instances_legacy.py: fix broken patches for new helpers (get_container_name removed, _ensure_backend_network_in_compose added, workspace_id/ssh_key_ids mock attributes added) - docker-compose.traefik.yml: add /data/working-copies bind mount Quality gates: pytest (19 passed, 1 skipped) --- apps/api/src/api/workspaces.py | 6 ++ apps/api/src/services/workspace_manager.py | 7 ++ .../tests/unit/test_tool_instances_legacy.py | 83 +++++++++++++------ docker-compose.traefik.yml | 6 +- 4 files changed, 75 insertions(+), 27 deletions(-) diff --git a/apps/api/src/api/workspaces.py b/apps/api/src/api/workspaces.py index a5526a8..acdee4d 100644 --- a/apps/api/src/api/workspaces.py +++ b/apps/api/src/api/workspaces.py @@ -232,6 +232,12 @@ async def create_workspace( workspace = await manager.create(repo, user_id, name, branch, session=session) session.add(workspace) await session.commit() + except HTTPException: + raise + except ValueError as exc: + await session.rollback() + logger.error("Failed to create workspace: %s", exc) + raise HTTPException(status_code=400, detail=str(exc)) from exc except Exception as exc: await session.rollback() logger.error("Failed to create workspace: %s", exc) diff --git a/apps/api/src/services/workspace_manager.py b/apps/api/src/services/workspace_manager.py index a9c5532..88932b3 100644 --- a/apps/api/src/services/workspace_manager.py +++ b/apps/api/src/services/workspace_manager.py @@ -88,6 +88,13 @@ class WorkspaceManager: if not repo.remote_url: raise ValueError("Repository has no remote URL") + # Remove stale directory from previous failed/aborted clone + if os.path.exists(path): + logger.warning( + "Removing stale workspace directory: %s", path + ) + shutil.rmtree(path, ignore_errors=True) + # Load SSH key if repo has one ssh_key = None if getattr(repo, "ssh_key_id", None) and session is not None: diff --git a/apps/api/tests/unit/test_tool_instances_legacy.py b/apps/api/tests/unit/test_tool_instances_legacy.py index 03f2655..a2a1d86 100644 --- a/apps/api/tests/unit/test_tool_instances_legacy.py +++ b/apps/api/tests/unit/test_tool_instances_legacy.py @@ -145,10 +145,12 @@ class TestCreateInstanceDockerfileLegacy: data = MagicMock() data.tool_type_id = str(fake_tool_type_id) data.display_name = None + data.workspace_id = None data.clone_mode = "mount" data.branch = None data.new_branch = None data.config_profile_id = None + data.ssh_key_ids = [] result = await create_instance( project_id=fake_project_id, @@ -225,10 +227,12 @@ class TestCreateInstanceDockerfileLegacy: data = MagicMock() data.tool_type_id = str(fake_tool_type_id) data.display_name = None + data.workspace_id = None data.clone_mode = "mount" data.branch = None data.new_branch = None data.config_profile_id = None + data.ssh_key_ids = [] with pytest.raises(HTTPException) as exc_info: await create_instance( @@ -305,10 +309,12 @@ class TestCreateInstanceComposeLegacy: data = MagicMock() data.tool_type_id = str(fake_tool_type_id) data.display_name = None + data.workspace_id = None data.clone_mode = "mount" data.branch = None data.new_branch = None data.config_profile_id = None + data.ssh_key_ids = [] result = await create_instance( project_id=fake_project_id, @@ -389,10 +395,12 @@ class TestCreateInstanceManifestNotCalledForLegacy: data = MagicMock() data.tool_type_id = str(fake_tool_type_id) data.display_name = None + data.workspace_id = None data.clone_mode = "mount" data.branch = None data.new_branch = None data.config_profile_id = None + data.ssh_key_ids = [] await create_instance( project_id=fake_project_id, @@ -411,8 +419,10 @@ class TestStartInstanceLegacyFallback: @patch("src.api.tool_instances.wait_for_container_running") @patch("src.api.tool_instances.execute_compose_command") @patch("src.api.tool_instances.get_container_id") - @patch("src.api.tool_instances.get_container_name") @patch("src.api.tool_instances.connect_container_to_network") + @patch("src.api.tool_instances._ensure_backend_network_in_compose") + @patch("src.api.tool_instances._ensure_container_name_in_compose") + @patch("src.api.tool_instances._ensure_web_bind_address") @patch("src.api.tool_instances._sanitize_compose_file") @patch("src.api.tool_instances._prepare_manifest_instance") @patch("src.api.tool_instances._get_user") @@ -423,8 +433,10 @@ class TestStartInstanceLegacyFallback: mock_get_user, mock_prepare_manifest, mock_sanitize, + mock_ensure_web_bind, + mock_ensure_container_name, + mock_backend_network, mock_connect_network, - mock_get_container_name, mock_get_container_id, mock_execute_compose, mock_wait_container, @@ -440,7 +452,6 @@ class TestStartInstanceLegacyFallback: mock_get_project.return_value = AsyncMock() mock_execute_compose.return_value = (0, "started", "") mock_get_container_id.return_value = "abc123" - mock_get_container_name.return_value = "test-container" mock_connect_network.return_value = True mock_wait_container.return_value = { "success": True, @@ -509,8 +520,10 @@ class TestStartInstanceLegacyFallback: @patch("src.api.tool_instances.wait_for_container_running") @patch("src.api.tool_instances.execute_compose_command") @patch("src.api.tool_instances.get_container_id") - @patch("src.api.tool_instances.get_container_name") @patch("src.api.tool_instances.connect_container_to_network") + @patch("src.api.tool_instances._ensure_backend_network_in_compose") + @patch("src.api.tool_instances._ensure_container_name_in_compose") + @patch("src.api.tool_instances._ensure_web_bind_address") @patch("src.api.tool_instances._sanitize_compose_file") @patch("src.api.tool_instances._prepare_manifest_instance") @patch("src.api.tool_instances._get_user") @@ -521,8 +534,10 @@ class TestStartInstanceLegacyFallback: mock_get_user, mock_prepare_manifest, mock_sanitize, + mock_ensure_web_bind, + mock_ensure_container_name, + mock_backend_network, mock_connect_network, - mock_get_container_name, mock_get_container_id, mock_execute_compose, mock_wait_container, @@ -538,7 +553,6 @@ class TestStartInstanceLegacyFallback: mock_get_project.return_value = AsyncMock() mock_execute_compose.return_value = (0, "started", "") mock_get_container_id.return_value = "abc123" - mock_get_container_name.return_value = "test-container" mock_connect_network.return_value = True mock_wait_container.return_value = { "success": True, @@ -606,8 +620,10 @@ class TestStartInstanceLegacyFallback: @patch("src.api.tool_instances.wait_for_container_running") @patch("src.api.tool_instances.execute_compose_command") @patch("src.api.tool_instances.get_container_id") - @patch("src.api.tool_instances.get_container_name") @patch("src.api.tool_instances.connect_container_to_network") + @patch("src.api.tool_instances._ensure_backend_network_in_compose") + @patch("src.api.tool_instances._ensure_container_name_in_compose") + @patch("src.api.tool_instances._ensure_web_bind_address") @patch("src.api.tool_instances._sanitize_compose_file") @patch("src.api.tool_instances._prepare_manifest_instance") @patch("src.api.tool_instances._get_user") @@ -618,8 +634,10 @@ class TestStartInstanceLegacyFallback: mock_get_user, mock_prepare_manifest, mock_sanitize, + mock_ensure_web_bind, + mock_ensure_container_name, + mock_backend_network, mock_connect_network, - mock_get_container_name, mock_get_container_id, mock_execute_compose, mock_wait_container, @@ -635,7 +653,6 @@ class TestStartInstanceLegacyFallback: mock_get_project.return_value = AsyncMock() mock_execute_compose.return_value = (0, "started", "") mock_get_container_id.return_value = "abc123" - mock_get_container_name.return_value = "test-container" mock_connect_network.return_value = True mock_wait_container.return_value = { "success": True, @@ -705,12 +722,15 @@ class TestStartInstanceSshPermissions: """SSH key mounts trigger permission fixes after container starts.""" @patch("src.api.tool_instances.write_compose_file") + @patch("src.api.tool_instances.prepare_ssh_key_files") @patch("src.api.tool_instances.apply_ssh_permissions") @patch("src.api.tool_instances.wait_for_container_running") @patch("src.api.tool_instances.execute_compose_command") @patch("src.api.tool_instances.get_container_id") - @patch("src.api.tool_instances.get_container_name") @patch("src.api.tool_instances.connect_container_to_network") + @patch("src.api.tool_instances._ensure_backend_network_in_compose") + @patch("src.api.tool_instances._ensure_container_name_in_compose") + @patch("src.api.tool_instances._ensure_web_bind_address") @patch("src.api.tool_instances._sanitize_compose_file") @patch("src.api.tool_instances._get_user") @patch("src.api.tool_instances._get_owned_project") @@ -719,12 +739,15 @@ class TestStartInstanceSshPermissions: mock_get_project, mock_get_user, mock_sanitize, + mock_ensure_web_bind, + mock_ensure_container_name, + mock_backend_network, mock_connect_network, - mock_get_container_name, mock_get_container_id, mock_execute_compose, mock_wait_container, mock_apply_ssh, + mock_prepare_ssh, mock_write_compose, mock_session, fake_user_id, @@ -743,7 +766,6 @@ class TestStartInstanceSshPermissions: mock_get_project.return_value = AsyncMock() mock_execute_compose.return_value = (0, "started", "") mock_get_container_id.return_value = "abc123" - mock_get_container_name.return_value = "test-container" mock_connect_network.return_value = True mock_wait_container.return_value = { "success": True, @@ -836,12 +858,15 @@ class TestStartInstanceSshPermissions: assert result["status"] == "running" mock_apply_ssh.assert_called_once_with("abc123", "/home/user/.ssh", "user") + @patch("src.api.tool_instances.prepare_ssh_key_files") @patch("src.api.tool_instances.apply_ssh_permissions") @patch("src.api.tool_instances.wait_for_container_running") @patch("src.api.tool_instances.execute_compose_command") @patch("src.api.tool_instances.get_container_id") - @patch("src.api.tool_instances.get_container_name") @patch("src.api.tool_instances.connect_container_to_network") + @patch("src.api.tool_instances._ensure_backend_network_in_compose") + @patch("src.api.tool_instances._ensure_container_name_in_compose") + @patch("src.api.tool_instances._ensure_web_bind_address") @patch("src.api.tool_instances._sanitize_compose_file") @patch("src.api.tool_instances._get_user") @patch("src.api.tool_instances._get_owned_project") @@ -850,12 +875,15 @@ class TestStartInstanceSshPermissions: mock_get_project, mock_get_user, mock_sanitize, + mock_ensure_web_bind, + mock_ensure_container_name, + mock_backend_network, mock_connect_network, - mock_get_container_name, mock_get_container_id, mock_execute_compose, mock_wait_container, mock_apply_ssh, + mock_prepare_ssh, mock_session, fake_user_id, fake_project_id, @@ -870,7 +898,6 @@ class TestStartInstanceSshPermissions: mock_get_project.return_value = AsyncMock() mock_execute_compose.return_value = (0, "started", "") mock_get_container_id.return_value = "abc123" - mock_get_container_name.return_value = "test-container" mock_connect_network.return_value = True mock_wait_container.return_value = { "success": True, @@ -933,14 +960,15 @@ class TestStartInstanceSshPermissions: mock_session.get.side_effect = _get with patch("os.path.exists", return_value=True): - result = await start_instance( - project_id=fake_project_id, - repo_id=fake_repo_id, - instance_id=fake_instance_id, - data=None, - user_id=fake_user_id, - session=mock_session, - ) + with patch("src.api.tool_instances._modify_compose_file"): + result = await start_instance( + project_id=fake_project_id, + repo_id=fake_repo_id, + instance_id=fake_instance_id, + data=None, + user_id=fake_user_id, + session=mock_session, + ) assert result["status"] == "running" mock_apply_ssh.assert_called_once_with("abc123", "/root/.ssh", "root") @@ -952,8 +980,10 @@ class TestStartInstanceManifestBranch: @patch("src.api.tool_instances.wait_for_container_running") @patch("src.api.tool_instances.execute_compose_command") @patch("src.api.tool_instances.get_container_id") - @patch("src.api.tool_instances.get_container_name") @patch("src.api.tool_instances.connect_container_to_network") + @patch("src.api.tool_instances._ensure_backend_network_in_compose") + @patch("src.api.tool_instances._ensure_container_name_in_compose") + @patch("src.api.tool_instances._ensure_web_bind_address") @patch("src.api.tool_instances._sanitize_compose_file") @patch("src.api.tool_instances._prepare_manifest_instance") @patch("src.api.tool_instances.write_compose_file") @@ -966,8 +996,10 @@ class TestStartInstanceManifestBranch: mock_write_compose, mock_prepare_manifest, mock_sanitize, + mock_ensure_web_bind, + mock_ensure_container_name, + mock_backend_network, mock_connect_network, - mock_get_container_name, mock_get_container_id, mock_execute_compose, mock_wait_container, @@ -987,7 +1019,6 @@ class TestStartInstanceManifestBranch: mock_get_project.return_value = AsyncMock() mock_execute_compose.return_value = (0, "started", "") mock_get_container_id.return_value = "abc123" - mock_get_container_name.return_value = "test-container" mock_connect_network.return_value = True mock_wait_container.return_value = { "success": True, diff --git a/docker-compose.traefik.yml b/docker-compose.traefik.yml index c4d5f10..32b1977 100644 --- a/docker-compose.traefik.yml +++ b/docker-compose.traefik.yml @@ -13,7 +13,11 @@ services: volumes: - postgres_data:/var/lib/postgresql/data healthcheck: - test: ["CMD-SHELL", "pg_isready -U ${POSTGRES_USER:-headquarter} -d ${POSTGRES_DB:-headquarter}"] + test: + [ + "CMD-SHELL", + "pg_isready -U ${POSTGRES_USER:-headquarter} -d ${POSTGRES_DB:-headquarter}", + ] interval: 10s timeout: 5s retries: 5