From 1b068260eb13cd96605f5f895a3dbdc58f3b3038 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 12 Jun 2026 07:00:30 +0000 Subject: [PATCH 1/2] Initial plan From 6233e23e6328472d879c10cf1d2a60b6050a64a4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 12 Jun 2026 07:26:20 +0000 Subject: [PATCH 2/2] Fix: MultipleResultsFound 500 error when retrying package installation (issue #42) --- .../versions/f1a2b3c4d5e6_unique_env_pkg.py | 48 +++++++++ backend/src/environments/models.py | 3 +- backend/src/environments/router.py | 14 +-- backend/src/environments/service.py | 35 +++++-- backend/src/environments/tasks.py | 4 +- backend/tests/environments/test_router.py | 97 +++++++++++++++++++ 6 files changed, 186 insertions(+), 15 deletions(-) create mode 100644 backend/migrations/versions/f1a2b3c4d5e6_unique_env_pkg.py diff --git a/backend/migrations/versions/f1a2b3c4d5e6_unique_env_pkg.py b/backend/migrations/versions/f1a2b3c4d5e6_unique_env_pkg.py new file mode 100644 index 00000000..3016cd62 --- /dev/null +++ b/backend/migrations/versions/f1a2b3c4d5e6_unique_env_pkg.py @@ -0,0 +1,48 @@ +"""add unique constraint on environment_packages(environment_id, package_name) + +Removes duplicate rows (keeping the highest-id record per pair) and then +adds a unique constraint so the MultipleResultsFound 500 error cannot recur. + +Revision ID: f1a2b3c4d5e6 +Revises: b4d2e1a9c3f7 +Create Date: 2026-06-12 08:00:00.000000 +""" +from typing import Sequence, Union + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = "f1a2b3c4d5e6" +down_revision: Union[str, None] = "b4d2e1a9c3f7" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + conn = op.get_bind() + + # Delete duplicate rows, keeping only the row with the highest id for each + # (environment_id, package_name) pair. Works on both SQLite and PostgreSQL. + conn.execute( + sa.text( + """ + DELETE FROM environment_packages + WHERE id NOT IN ( + SELECT MAX(id) + FROM environment_packages + GROUP BY environment_id, package_name + ) + """ + ) + ) + + with op.batch_alter_table("environment_packages") as batch_op: + batch_op.create_unique_constraint( + "uq_env_pkg", ["environment_id", "package_name"] + ) + + +def downgrade() -> None: + with op.batch_alter_table("environment_packages") as batch_op: + batch_op.drop_constraint("uq_env_pkg", type_="unique") diff --git a/backend/src/environments/models.py b/backend/src/environments/models.py index e0cf24a7..016a5567 100644 --- a/backend/src/environments/models.py +++ b/backend/src/environments/models.py @@ -2,7 +2,7 @@ from datetime import datetime -from sqlalchemy import Boolean, DateTime, ForeignKey, Integer, String, Text +from sqlalchemy import Boolean, DateTime, ForeignKey, Integer, String, Text, UniqueConstraint from sqlalchemy.orm import Mapped, mapped_column from src.database import Base, TimestampMixin @@ -36,6 +36,7 @@ class EnvironmentPackage(Base): """Installed package in an environment.""" __tablename__ = "environment_packages" + __table_args__ = (UniqueConstraint("environment_id", "package_name", name="uq_env_pkg"),) id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) environment_id: Mapped[int] = mapped_column(ForeignKey("environments.id", ondelete="CASCADE"), index=True) diff --git a/backend/src/environments/router.py b/backend/src/environments/router.py index 9ca2d907..841ec1c1 100644 --- a/backend/src/environments/router.py +++ b/backend/src/environments/router.py @@ -476,6 +476,10 @@ def install_package( pkg = add_package(db, env_id, data) + # Commit before dispatching so the background thread's separate session + # can see the new record (per CLAUDE.md critical pattern). + db.commit() + # Trigger async installation try: from src.environments.tasks import install_package @@ -504,20 +508,18 @@ def upgrade_package( raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Environment not found") # Find existing package record - result = db.execute( + pkg = db.execute( select(EnvironmentPackage).where( EnvironmentPackage.environment_id == env_id, EnvironmentPackage.package_name == package_name, ) - ) - pkg = result.scalar_one_or_none() + ).scalars().first() if pkg is None: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Package not found") # Clear version constraint and trigger upgrade pkg.version = None - db.flush() - db.refresh(pkg) + db.commit() try: from src.environments.tasks import upgrade_package @@ -550,7 +552,7 @@ def retry_package_install( EnvironmentPackage.environment_id == env_id, EnvironmentPackage.package_name == package_name, ) - ).scalar_one_or_none() + ).scalars().first() if pkg is None: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Package not found") diff --git a/backend/src/environments/service.py b/backend/src/environments/service.py index a2c563dc..959531fc 100644 --- a/backend/src/environments/service.py +++ b/backend/src/environments/service.py @@ -174,7 +174,29 @@ def list_packages(db: Session, env_id: int) -> list[EnvironmentPackage]: def add_package(db: Session, env_id: int, data: PackageCreate) -> EnvironmentPackage: - """Add a package to an environment.""" + """Add a package to an environment. + + If the package already exists (e.g. from a previous failed install), + the existing record is reset to 'pending' so it can be retried without + creating a duplicate row — which would cause MultipleResultsFound errors + in subsequent retry/upgrade queries. + """ + existing = db.execute( + select(EnvironmentPackage).where( + EnvironmentPackage.environment_id == env_id, + EnvironmentPackage.package_name == data.package_name, + ) + ).scalars().first() + + if existing is not None: + existing.version = data.version + existing.install_status = "pending" + existing.install_error = None + existing.installed_version = None + db.flush() + db.refresh(existing) + return existing + pkg = EnvironmentPackage( environment_id=env_id, package_name=data.package_name, @@ -188,16 +210,17 @@ def add_package(db: Session, env_id: int, data: PackageCreate) -> EnvironmentPac def remove_package(db: Session, env_id: int, package_name: str) -> None: """Remove a package from an environment.""" - result = db.execute( + # Use .all() defensively: the unique constraint guarantees at most one row, + # but pre-migration databases may still have duplicates that need cleaning up. + pkgs = db.execute( select(EnvironmentPackage).where( EnvironmentPackage.environment_id == env_id, EnvironmentPackage.package_name == package_name, ) - ) - pkg = result.scalar_one_or_none() - if pkg: + ).scalars().all() + for pkg in pkgs: db.delete(pkg) - db.flush() + db.flush() # --- Variables --- diff --git a/backend/src/environments/tasks.py b/backend/src/environments/tasks.py index 040f1f0c..502bd323 100644 --- a/backend/src/environments/tasks.py +++ b/backend/src/environments/tasks.py @@ -408,7 +408,7 @@ def _install_package_inner(env_id: int, package_name: str, version: str | None = EnvironmentPackage.environment_id == env_id, EnvironmentPackage.package_name == package_name, ) - ).scalar_one_or_none() + ).scalars().first() if pkg: pkg.install_status = "installing" @@ -542,7 +542,7 @@ def _upgrade_package_inner(env_id: int, package_name: str) -> dict: EnvironmentPackage.environment_id == env_id, EnvironmentPackage.package_name == package_name, ) - ).scalar_one_or_none() + ).scalars().first() if pkg: pkg.install_status = "installing" diff --git a/backend/tests/environments/test_router.py b/backend/tests/environments/test_router.py index 56c0068a..eafba1c4 100644 --- a/backend/tests/environments/test_router.py +++ b/backend/tests/environments/test_router.py @@ -1,6 +1,7 @@ """Tests for environment management API endpoints.""" import pytest +from sqlalchemy import select, text from unittest.mock import patch, MagicMock from src.environments.models import Environment, EnvironmentPackage, EnvironmentVariable @@ -746,6 +747,102 @@ def test_retry_endpoint_package_not_found(self, mock_dispatch, client, db_sessio ) assert response.status_code == 404 + @patch("src.environments.router.dispatch_task") + def test_install_package_twice_reuses_existing_record(self, mock_dispatch, client, db_session, admin_user): + """Installing the same package a second time must not create a duplicate row. + + Previously a second POST would create a new EnvironmentPackage row; then + retry / upgrade queries using scalar_one_or_none() would raise + MultipleResultsFound → 500 Internal Server Error (GitHub #42). + """ + mock_dispatch.return_value = MagicMock(id="fake-task-id") + + env = Environment( + name="dedup-env", + python_version="3.12", + created_by=admin_user.id, + ) + db_session.add(env) + db_session.flush() + db_session.refresh(env) + + # First install — creates the row + resp1 = client.post( + f"{URL}/{env.id}/packages", + json={"package_name": "requests", "version": "2.31.0"}, + headers=auth_header(admin_user), + ) + assert resp1.status_code == 201 + pkg_id_first = resp1.json()["id"] + + # Simulate a failed install so the user would try again + db_session.execute( + text("UPDATE environment_packages SET install_status='failed' WHERE id=:id"), + {"id": pkg_id_first}, + ) + db_session.commit() + + # Second install of the same package — must reuse the existing row + resp2 = client.post( + f"{URL}/{env.id}/packages", + json={"package_name": "requests", "version": "2.32.0"}, + headers=auth_header(admin_user), + ) + assert resp2.status_code == 201 + pkg_id_second = resp2.json()["id"] + + # Same DB row, not a new one + assert pkg_id_first == pkg_id_second + + # Status reset to pending, version updated + assert resp2.json()["install_status"] == "pending" + assert resp2.json()["version"] == "2.32.0" + + # Exactly one row in DB for this package + rows = db_session.execute( + select(EnvironmentPackage).where( + EnvironmentPackage.environment_id == env.id, + EnvironmentPackage.package_name == "requests", + ) + ).scalars().all() + assert len(rows) == 1 + + @patch("src.environments.router.dispatch_task") + def test_retry_after_failed_install_does_not_500(self, mock_dispatch, client, db_session, admin_user): + """Retry on a failed package (the scenario from GitHub #42) must succeed. + + The root cause was scalar_one_or_none() raising MultipleResultsFound when + duplicate rows existed. With the upsert fix + unique constraint this can no + longer happen, but the endpoint should work correctly in all cases. + """ + mock_dispatch.return_value = MagicMock(id="fake-task-id") + + env = Environment( + name="retry-ok-env", + python_version="3.12", + created_by=admin_user.id, + ) + db_session.add(env) + db_session.flush() + db_session.refresh(env) + + pkg = EnvironmentPackage( + environment_id=env.id, + package_name="robotframework", + install_status="failed", + install_error="some error", + ) + db_session.add(pkg) + db_session.flush() + + response = client.post( + f"{URL}/{env.id}/packages/robotframework/retry", + headers=auth_header(admin_user), + ) + assert response.status_code == 200 + assert response.json()["install_status"] == "pending" + assert response.json()["install_error"] is None + class TestRfLibraries: """Tests for the RF keyword library scan endpoint."""