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)
2026-06-02 13:41:07 +02:00
parent e1aaf9f6fc
commit 906aab3b73
4 changed files with 75 additions and 27 deletions
@@ -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)
@@ -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:
@@ -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,
@@ -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