refactor: per-operation DB sessions in Project/Mcp services

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

src/hqt/mcp/service.py

@@ -1,13 +1,15 @@
 import json
 
-from sqlalchemy.orm import Session as DBSession
+from sqlalchemy.orm import sessionmaker
 
 from hqt.db.models import McpServer, ProjectMcpServer
 
 
 class McpService:
-    def __init__(self, db: DBSession):
+    def __init__(self, factory: sessionmaker):
-        self.db = db
+        # factory must be created with expire_on_commit=False so rows returned
+        # from these methods stay readable after their session closes.
+        self.factory = factory
 
     def create(
         self,
@@ -17,6 +19,7 @@ class McpService:
         args: list[str] | None = None,
         url: str | None = None,
     ) -> McpServer:
+        with self.factory() as db:
             server = McpServer(
                 name=name,
                 transport=transport,
@@ -24,42 +27,45 @@ class McpService:
                 args_json=json.dumps(args) if args else None,
                 url=url,
             )
-        self.db.add(server)
+            db.add(server)
-        self.db.commit()
+            db.commit()
-        self.db.refresh(server)
             return server
 
     def list_all(self) -> list[McpServer]:
-        return list(self.db.query(McpServer).all())
+        with self.factory() as db:
+            return list(db.query(McpServer).all())
 
     def get(self, name: str) -> McpServer | None:
-        return self.db.query(McpServer).filter_by(name=name).first()
+        with self.factory() as db:
+            return db.query(McpServer).filter_by(name=name).first()
 
     def delete(self, name: str) -> None:
-        server = self.get(name)
+        with self.factory() as db:
+            server = db.query(McpServer).filter_by(name=name).first()
             if server:
-            self.db.delete(server)
+                db.delete(server)
-            self.db.commit()
+                db.commit()
 
     def bind_to_project(self, project_id: int, mcp_server_id: int) -> None:
-        self.db.add(
+        with self.factory() as db:
-            ProjectMcpServer(project_id=project_id, mcp_server_id=mcp_server_id)
+            db.add(ProjectMcpServer(project_id=project_id, mcp_server_id=mcp_server_id))
-        )
+            db.commit()
-        self.db.commit()
 
     def unbind_from_project(self, project_id: int, mcp_server_id: int) -> None:
-        self.db.query(ProjectMcpServer).filter_by(
+        with self.factory() as db:
+            db.query(ProjectMcpServer).filter_by(
                 project_id=project_id, mcp_server_id=mcp_server_id
             ).delete()
-        self.db.commit()
+            db.commit()
 
     def get_project_mcps(self, project_id: int) -> list[McpServer]:
+        with self.factory() as db:
             ids = [
                 r.mcp_server_id
-            for r in self.db.query(ProjectMcpServer)
+                for r in db.query(ProjectMcpServer)
                 .filter_by(project_id=project_id)
                 .all()
             ]
             if not ids:
                 return []
-        return list(self.db.query(McpServer).filter(McpServer.id.in_(ids)).all())
+            return list(db.query(McpServer).filter(McpServer.id.in_(ids)).all())

src/hqt/projects/service.py

@@ -1,45 +1,55 @@
 from sqlalchemy.exc import IntegrityError
-from sqlalchemy.orm import Session as DBSession
+from sqlalchemy.orm import sessionmaker
 
 from hqt.db.models import Project
+from hqt.errors import ServiceError
 
 
 class ProjectService:
-    def __init__(self, db: DBSession):
+    def __init__(self, factory: sessionmaker):
-        self.db = db
+        # factory must be created with expire_on_commit=False so rows returned
+        # from these methods stay readable after their session closes.
+        self.factory = factory
 
     def create(self, name: str, path: str) -> Project:
+        with self.factory() as db:
             project = Project(name=name, path=path)
-        self.db.add(project)
+            db.add(project)
-        self.db.commit()
+            try:
-        self.db.refresh(project)
+                db.commit()
+            except IntegrityError as err:
+                db.rollback()
+                raise ServiceError(f"Another project already uses path {path}") from err
             return project
 
     def list_all(self, include_archived: bool = False) -> list[Project]:
-        q = self.db.query(Project)
+        with self.factory() as db:
+            q = db.query(Project)
             if not include_archived:
                 q = q.filter_by(archived=False)
             return list(q.all())
 
     def get(self, project_id: int) -> Project | None:
-        return self.db.get(Project, project_id)
+        with self.factory() as db:
+            return db.get(Project, project_id)
 
     def update(self, project_id: int, name: str, path: str) -> Project:
-        project = self.get(project_id)
+        with self.factory() as db:
+            project = db.get(Project, project_id)
             if project is None:
-            raise ValueError(f"Project {project_id} not found")
+                raise ServiceError(f"Project {project_id} not found")
             project.name = name
             project.path = path
             try:
-            self.db.commit()
+                db.commit()
             except IntegrityError as err:
-            self.db.rollback()
+                db.rollback()
-            raise ValueError(f"Another project already uses path {path}") from err
+                raise ServiceError(f"Another project already uses path {path}") from err
-        self.db.refresh(project)
             return project
 
     def archive(self, project_id: int) -> None:
-        project = self.get(project_id)
+        with self.factory() as db:
+            project = db.get(Project, project_id)
             if project:
                 project.archived = True
-            self.db.commit()
+                db.commit()

tests/test_services.py

@@ -1,62 +1,76 @@
 import pytest
 from sqlalchemy import create_engine
-from sqlalchemy.orm import Session
+from sqlalchemy.orm import sessionmaker
+from sqlalchemy.pool import StaticPool
 
 from hqt.db.models import Base
+from hqt.errors import ServiceError
 from hqt.mcp.service import McpService
 from hqt.projects.service import ProjectService
 
 
 @pytest.fixture
-def db():
+def factory():
-    engine = create_engine("sqlite:///:memory:")
+    # StaticPool + check_same_thread=False: every session created by the
+    # factory shares the single in-memory connection, so per-operation
+    # sessions all see the same database.
+    engine = create_engine(
+        "sqlite://",
+        connect_args={"check_same_thread": False},
+        poolclass=StaticPool,
+    )
     Base.metadata.create_all(engine)
-    with Session(engine) as session:
+    return sessionmaker(bind=engine, expire_on_commit=False)
-        yield session
 
 
 class TestProjectService:
-    def test_create_and_get(self, db):
+    def test_create_and_get(self, factory):
-        svc = ProjectService(db)
+        svc = ProjectService(factory)
         p = svc.create("test", "/tmp/test")
         assert p.id is not None
         assert svc.get(p.id).name == "test"
 
-    def test_list_excludes_archived(self, db):
+    def test_list_excludes_archived(self, factory):
-        svc = ProjectService(db)
+        svc = ProjectService(factory)
         svc.create("a", "/a")
         p2 = svc.create("b", "/b")
         svc.archive(p2.id)
         assert len(svc.list_all()) == 1
         assert len(svc.list_all(include_archived=True)) == 2
 
-    def test_update_name_and_path(self, db):
+    def test_update_name_and_path(self, factory):
-        svc = ProjectService(db)
+        svc = ProjectService(factory)
         p = svc.create("old", "/old/path")
         updated = svc.update(p.id, "new", "/new/path")
         assert updated.name == "new"
         assert updated.path == "/new/path"
         assert svc.get(p.id).path == "/new/path"
 
-    def test_update_duplicate_path_raises(self, db):
+    def test_update_duplicate_path_raises(self, factory):
-        svc = ProjectService(db)
+        svc = ProjectService(factory)
         svc.create("a", "/a")
         p2 = svc.create("b", "/b")
-        with pytest.raises(ValueError, match="already uses path"):
+        with pytest.raises(ServiceError, match="already uses path"):
             svc.update(p2.id, "b", "/a")
         # DB session must remain usable after rollback, values unchanged
         assert svc.get(p2.id).path == "/b"
         assert svc.get(p2.id).name == "b"
 
-    def test_update_unknown_id_raises(self, db):
+    def test_update_unknown_id_raises(self, factory):
-        svc = ProjectService(db)
+        svc = ProjectService(factory)
-        with pytest.raises(ValueError, match="not found"):
+        with pytest.raises(ServiceError, match="not found"):
             svc.update(9999, "x", "/x")
 
+    def test_create_duplicate_path_raises_service_error(self, factory):
+        svc = ProjectService(factory)
+        svc.create("a", "/dup")
+        with pytest.raises(ServiceError, match="already uses path"):
+            svc.create("b", "/dup")
+
 
 class TestMcpService:
-    def test_crud(self, db):
+    def test_crud(self, factory):
-        svc = McpService(db)
+        svc = McpService(factory)
         server = svc.create("test-mcp", "stdio", command="node", args=["server.js"])
         assert server.id is not None
         assert svc.get("test-mcp") is not None
@@ -64,9 +78,9 @@ class TestMcpService:
         svc.delete("test-mcp")
         assert svc.get("test-mcp") is None
 
-    def test_bind_unbind(self, db):
+    def test_bind_unbind(self, factory):
-        psvc = ProjectService(db)
+        psvc = ProjectService(factory)
-        msvc = McpService(db)
+        msvc = McpService(factory)
         project = psvc.create("proj", "/proj")
         server = msvc.create("s1", "stdio", command="cmd")
         msvc.bind_to_project(project.id, server.id)