diff --git a/features/steps/tdd_data_integrity_session_rollback_7489_steps.py b/features/steps/tdd_data_integrity_session_rollback_7489_steps.py new file mode 100644 index 000000000..7c53f77a6 --- /dev/null +++ b/features/steps/tdd_data_integrity_session_rollback_7489_steps.py @@ -0,0 +1,686 @@ +"""Steps for testing data integrity with shared UoW sessions. + +Tests that repositories do NOT call rollback() on shared sessions, +which would roll back all pending work from other repositories. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +from behave import given, then, when +from sqlalchemy.orm import Session + +from cleveragents.core.exceptions import DatabaseError +from cleveragents.domain.models.core import ( + Actor, + Change, + Context, + DebugAttempt, + Plan, + Project, + ProjectSettings, +) +from cleveragents.infrastructure.database.repositories import ( + ActorRepository, + ChangeRepository, + ContextRepository, + DebugAttemptRepository, + PlanRepository, + ProjectRepository, +) +from cleveragents.infrastructure.database.unit_of_work import UnitOfWork + + +@given("I have a clean in-memory SQLite database") +def step_clean_database(context: Any) -> None: + """Create a clean in-memory SQLite database.""" + context.db_url = "sqlite:///:memory:" + context.uow = UnitOfWork(context.db_url, require_confirmation=False) + + +@given("I have initialized the database schema") +def step_init_database(context: Any) -> None: + """Initialize the database schema.""" + context.uow.init_database() + + +@given("I have a UnitOfWork with a shared session") +def step_uow_shared_session(context: Any) -> None: + """Create a UnitOfWork and get a shared session.""" + if not hasattr(context, "uow"): + context.db_url = "sqlite:///:memory:" + context.uow = UnitOfWork(context.db_url, require_confirmation=False) + context.uow.init_database() + + # Create a shared session for the test + context.shared_session = context.uow.session_factory() + context.created_objects: dict[str, Any] = {} + context.errors: list[Exception] = [] + + +@given("I have a ProjectRepository using the shared session") +def step_project_repo_shared(context: Any) -> None: + """Create a ProjectRepository with the shared session.""" + context.project_repo = ProjectRepository(context.shared_session) + + +@given("I have an ActionRepository using the session factory") +def step_action_repo_factory(context: Any) -> None: + """Create an ActionRepository with the session factory.""" + from cleveragents.infrastructure.database.repositories import ActionRepository + + context.action_repo = ActionRepository(context.uow.session_factory) + + +@given("I have a ContextRepository using the shared session") +def step_context_repo_shared(context: Any) -> None: + """Create a ContextRepository with the shared session.""" + context.context_repo = ContextRepository(context.shared_session) + + +@given("I have a ChangeRepository using the shared session") +def step_change_repo_shared(context: Any) -> None: + """Create a ChangeRepository with the shared session.""" + context.change_repo = ChangeRepository(context.shared_session) + + +@given("I have a DebugAttemptRepository using the shared session") +def step_debug_attempt_repo_shared(context: Any) -> None: + """Create a DebugAttemptRepository with the shared session.""" + context.debug_attempt_repo = DebugAttemptRepository(context.shared_session) + + +@given("I have an ActorRepository using the shared session") +def step_actor_repo_shared(context: Any) -> None: + """Create an ActorRepository with the shared session.""" + context.actor_repo = ActorRepository(context.shared_session) + + +@given("I have a PlanRepository using the shared session") +def step_plan_repo_shared(context: Any) -> None: + """Create a PlanRepository with the shared session.""" + context.plan_repo = PlanRepository(context.shared_session) + + +@when("I create an action in the shared session") +def step_create_action_shared(context: Any) -> None: + """Create an action using the session factory.""" + from cleveragents.domain.models.core.action import Action, ActionNamespace + + action = Action( + namespaced_name=ActionNamespace(namespace="test", name="test-action"), + description="Test action", + long_description="Test action description", + definition_of_done="Test DoD", + strategy_actor="test-actor", + execution_actor="test-actor", + estimation_actor="test-actor", + review_actor="test-actor", + ) + context.created_objects["action"] = action + + +@when("I flush the action to the database") +def step_flush_action(context: Any) -> None: + """Flush the action to the database.""" + if "action" in context.created_objects: + action = context.created_objects["action"] + try: + context.action_repo.create(action) + context.shared_session.flush() + except Exception as e: + context.errors.append(e) + + +@when("I create a project in the shared session") +def step_create_project_shared(context: Any) -> None: + """Create a project in the shared session.""" + project = Project( + name="test-project", + path=Path("/tmp/test-project"), + settings=ProjectSettings(), + ) + context.created_objects["project"] = project + try: + context.project_repo.create(project) + context.shared_session.flush() + except Exception as e: + context.errors.append(e) + + +@when("I flush the project to the database") +def step_flush_project(context: Any) -> None: + """Flush the project to the database.""" + if "project" in context.created_objects: + context.shared_session.flush() + + +@when("I add context items to the shared session") +def step_add_context_shared(context: Any) -> None: + """Add context items to the shared session.""" + context_item = Context( + plan_id=1, + type="file", + path="/tmp/test.txt", + content="test content", + file_hash="abc123", + size=12, + ) + context.created_objects["context"] = context_item + try: + context.context_repo.add(context_item) + context.shared_session.flush() + except Exception as e: + context.errors.append(e) + + +@when("I flush the context to the database") +def step_flush_context(context: Any) -> None: + """Flush the context to the database.""" + if "context" in context.created_objects: + context.shared_session.flush() + + +@when("I add a change to the shared session") +def step_add_change_shared(context: Any) -> None: + """Add a change to the shared session.""" + change = Change( + plan_id=1, + file_path="/tmp/test.txt", + operation="create", + original_content="", + new_content="test content", + ) + context.created_objects["change"] = change + try: + context.change_repo.add(change) + context.shared_session.flush() + except Exception as e: + context.errors.append(e) + + +@when("I flush the change to the database") +def step_flush_change(context: Any) -> None: + """Flush the change to the database.""" + if "change" in context.created_objects: + context.shared_session.flush() + + +@when("I add a debug attempt to the shared session") +def step_add_debug_attempt_shared(context: Any) -> None: + """Add a debug attempt to the shared session.""" + from datetime import datetime + + debug_attempt = DebugAttempt( + plan_id=1, + error_message="Test error", + attempted_fix="Test fix", + success=False, + attempt_number=1, + created_at=datetime.now(), + ) + context.created_objects["debug_attempt"] = debug_attempt + try: + context.debug_attempt_repo.add(debug_attempt) + context.shared_session.flush() + except Exception as e: + context.errors.append(e) + + +@when("I flush the debug attempt to the database") +def step_flush_debug_attempt(context: Any) -> None: + """Flush the debug attempt to the database.""" + if "debug_attempt" in context.created_objects: + context.shared_session.flush() + + +@when("I upsert an actor in the shared session") +def step_upsert_actor_shared(context: Any) -> None: + """Upsert an actor in the shared session.""" + actor = Actor( + name="test-actor", + provider="test", + model="test-model", + config_blob={}, + config_hash="abc123", + yaml_text="test: yaml", + ) + context.created_objects["actor"] = actor + try: + context.actor_repo.upsert(actor) + context.shared_session.flush() + except Exception as e: + context.errors.append(e) + + +@when("I flush the actor to the database") +def step_flush_actor(context: Any) -> None: + """Flush the actor to the database.""" + if "actor" in context.created_objects: + context.shared_session.flush() + + +@when("I create a plan in the shared session") +def step_create_plan_shared(context: Any) -> None: + """Create a plan in the shared session.""" + plan = Plan( + project_id=1, + name="test-plan", + prompt="test prompt", + status="pending", + current=False, + ) + context.created_objects["plan"] = plan + try: + context.plan_repo.create(plan) + context.shared_session.flush() + except Exception as e: + context.errors.append(e) + + +@when("I flush the plan to the database") +def step_flush_plan(context: Any) -> None: + """Flush the plan to the database.""" + if "plan" in context.created_objects: + context.shared_session.flush() + + +@when("I attempt to create a project with invalid data that causes a database error") +def step_create_project_invalid(context: Any) -> None: + """Attempt to create a project with invalid data.""" + # Create a project with a very long name that exceeds the database column limit + # This will cause a database error + project = Project( + name="x" * 10000, # Extremely long name + path=Path("/tmp/test-project"), + settings=ProjectSettings(), + ) + context.created_objects["invalid_project"] = project + try: + context.project_repo.create(project) + context.shared_session.flush() + except (DatabaseError, Exception) as e: + context.errors.append(e) + context.last_error = e + + +@when("I attempt to create a duplicate action") +def step_create_duplicate_action(context: Any) -> None: + """Attempt to create a duplicate action.""" + from cleveragents.domain.models.core.action import Action, ActionNamespace + from cleveragents.infrastructure.database.repositories import DuplicateActionError + + # Create the same action again + action = Action( + namespaced_name=ActionNamespace(namespace="test", name="test-action"), + description="Test action", + long_description="Test action description", + definition_of_done="Test DoD", + strategy_actor="test-actor", + execution_actor="test-actor", + estimation_actor="test-actor", + review_actor="test-actor", + ) + try: + context.action_repo.create(action) + except DuplicateActionError as e: + context.last_error = e + context.errors.append(e) + + +@when("I attempt to add another change with invalid data") +def step_add_invalid_change(context: Any) -> None: + """Attempt to add a change with invalid data.""" + change = Change( + plan_id=1, + file_path="x" * 10000, # Extremely long path + operation="create", + original_content="", + new_content="test content", + ) + context.created_objects["invalid_change"] = change + try: + context.change_repo.add(change) + context.shared_session.flush() + except (DatabaseError, Exception) as e: + context.errors.append(e) + context.last_error = e + + +@when("I attempt to create a project with invalid data") +def step_create_invalid_project(context: Any) -> None: + """Attempt to create a project with invalid data.""" + project = Project( + name="x" * 10000, # Extremely long name + path=Path("/tmp/test-project"), + settings=ProjectSettings(), + ) + context.created_objects["invalid_project"] = project + try: + context.project_repo.create(project) + context.shared_session.flush() + except (DatabaseError, Exception) as e: + context.errors.append(e) + context.last_error = e + + +@when("I attempt to create another project with invalid data") +def step_create_another_invalid_project(context: Any) -> None: + """Attempt to create another project with invalid data.""" + project = Project( + name="y" * 10000, # Extremely long name + path=Path("/tmp/test-project-2"), + settings=ProjectSettings(), + ) + context.created_objects["invalid_project_2"] = project + try: + context.project_repo.create(project) + context.shared_session.flush() + except (DatabaseError, Exception) as e: + context.errors.append(e) + context.last_error = e + + +@when("I attempt to upsert another actor with invalid data") +def step_upsert_invalid_actor(context: Any) -> None: + """Attempt to upsert an actor with invalid data.""" + actor = Actor( + name="x" * 10000, # Extremely long name + provider="test", + model="test-model", + config_blob={}, + config_hash="abc123", + yaml_text="test: yaml", + ) + context.created_objects["invalid_actor"] = actor + try: + context.actor_repo.upsert(actor) + context.shared_session.flush() + except (DatabaseError, Exception) as e: + context.errors.append(e) + context.last_error = e + + +@when("I attempt to create a plan with invalid data") +def step_create_invalid_plan(context: Any) -> None: + """Attempt to create a plan with invalid data.""" + plan = Plan( + project_id=1, + name="x" * 10000, # Extremely long name + prompt="test prompt", + status="pending", + current=False, + ) + context.created_objects["invalid_plan"] = plan + try: + context.plan_repo.create(plan) + context.shared_session.flush() + except (DatabaseError, Exception) as e: + context.errors.append(e) + context.last_error = e + + +@when("I create an action using the session factory") +def step_create_action_factory(context: Any) -> None: + """Create an action using the session factory.""" + from cleveragents.domain.models.core.action import Action, ActionNamespace + + action = Action( + namespaced_name=ActionNamespace(namespace="test", name="test-action"), + description="Test action", + long_description="Test action description", + definition_of_done="Test DoD", + strategy_actor="test-actor", + execution_actor="test-actor", + estimation_actor="test-actor", + review_actor="test-actor", + ) + context.created_objects["action"] = action + try: + context.action_repo.create(action) + except Exception as e: + context.errors.append(e) + + +@when("I flush the action to the database") +def step_flush_action_factory(context: Any) -> None: + """Flush the action to the database.""" + # Session factory repositories manage their own sessions + pass + + +@then("the action should still be persisted in the database") +def step_action_persisted(context: Any) -> None: + """Verify the action is persisted.""" + # Commit the shared session to persist the action + try: + context.shared_session.commit() + except Exception: + pass + + # Query the database to verify the action exists + from cleveragents.infrastructure.database.models import LifecycleActionModel + + session = context.uow.session_factory() + try: + action = ( + session.query(LifecycleActionModel) + .filter_by(namespaced_name="test/test-action") + .first() + ) + assert action is not None, "Action should be persisted in the database" + finally: + session.close() + + +@then("the database error should be raised") +def step_error_raised(context: Any) -> None: + """Verify that a database error was raised.""" + assert len(context.errors) > 0, "A database error should have been raised" + assert isinstance(context.errors[-1], (DatabaseError, Exception)), ( + "The error should be a DatabaseError or Exception" + ) + + +@then("the shared session should NOT be rolled back by the repository") +def step_session_not_rolled_back(context: Any) -> None: + """Verify the shared session was not rolled back by the repository.""" + # The repository should NOT call rollback() on the shared session + # This is verified by checking that the session is still in a valid state + # and that previously flushed data is still in the session + assert context.shared_session.is_active, "The shared session should still be active" + + +@then("the first project should be persisted") +def step_first_project_persisted(context: Any) -> None: + """Verify the first project is persisted.""" + try: + context.shared_session.commit() + except Exception: + pass + + from cleveragents.infrastructure.database.models import ProjectModel + + session = context.uow.session_factory() + try: + project = session.query(ProjectModel).filter_by(name="test-project").first() + assert project is not None, "First project should be persisted" + finally: + session.close() + + +@then("the context items should be persisted") +def step_context_persisted(context: Any) -> None: + """Verify context items are persisted.""" + try: + context.shared_session.commit() + except Exception: + pass + + from cleveragents.infrastructure.database.models import ContextModel + + session = context.uow.session_factory() + try: + context_item = session.query(ContextModel).filter_by(plan_id=1).first() + assert context_item is not None, "Context items should be persisted" + finally: + session.close() + + +@then("the second project creation should fail") +def step_second_project_fails(context: Any) -> None: + """Verify the second project creation failed.""" + assert len(context.errors) > 0, "The second project creation should have failed" + + +@then("the UnitOfWork should handle the rollback") +def step_uow_handles_rollback(context: Any) -> None: + """Verify the UnitOfWork handles the rollback.""" + # The UnitOfWork should be responsible for rollback, not the repository + # This is verified by checking that the error was raised + assert len(context.errors) > 0, "An error should have been raised" + + +@then("the repository should NOT call rollback() on the shared session") +def step_repo_no_rollback(context: Any) -> None: + """Verify the repository did not call rollback().""" + # The repository should not call rollback() on the shared session + # This is verified by checking that the session is still active + assert context.shared_session.is_active, "The shared session should still be active" + + +@then("the error should be propagated to the caller") +def step_error_propagated(context: Any) -> None: + """Verify the error was propagated.""" + assert len(context.errors) > 0, "The error should have been propagated" + + +@then("the ActionRepository should rollback its own session") +def step_action_repo_rollback(context: Any) -> None: + """Verify the ActionRepository rolled back its own session.""" + # The ActionRepository uses a session factory and creates its own sessions + # It should be able to call rollback() on those sessions + # This is verified by checking that the duplicate action was not persisted + pass + + +@then("the duplicate action should not be persisted") +def step_duplicate_not_persisted(context: Any) -> None: + """Verify the duplicate action was not persisted.""" + from cleveragents.infrastructure.database.models import LifecycleActionModel + + session = context.uow.session_factory() + try: + # Count the number of actions with the same name + count = ( + session.query(LifecycleActionModel) + .filter_by(namespaced_name="test/test-action") + .count() + ) + # Should only have one (the first one created) + assert count == 1, "Only one action should be persisted" + finally: + session.close() + + +@then("a DuplicateActionError should be raised") +def step_duplicate_error_raised(context: Any) -> None: + """Verify a DuplicateActionError was raised.""" + from cleveragents.infrastructure.database.repositories import DuplicateActionError + + assert len(context.errors) > 0, "An error should have been raised" + assert isinstance(context.errors[-1], DuplicateActionError), ( + "A DuplicateActionError should have been raised" + ) + + +@then("the first change should be persisted") +def step_first_change_persisted(context: Any) -> None: + """Verify the first change is persisted.""" + try: + context.shared_session.commit() + except Exception: + pass + + from cleveragents.infrastructure.database.models import ChangeModel + + session = context.uow.session_factory() + try: + change = session.query(ChangeModel).filter_by(plan_id=1).first() + assert change is not None, "First change should be persisted" + finally: + session.close() + + +@then("the debug attempt should be persisted") +def step_debug_attempt_persisted(context: Any) -> None: + """Verify the debug attempt is persisted.""" + try: + context.shared_session.commit() + except Exception: + pass + + from cleveragents.infrastructure.database.models import DebugAttemptModel + + session = context.uow.session_factory() + try: + debug_attempt = session.query(DebugAttemptModel).filter_by(plan_id=1).first() + assert debug_attempt is not None, "Debug attempt should be persisted" + finally: + session.close() + + +@then("the second change creation should fail") +def step_second_change_fails(context: Any) -> None: + """Verify the second change creation failed.""" + assert len(context.errors) > 0, "The second change creation should have failed" + + +@then("the first actor should be persisted") +def step_first_actor_persisted(context: Any) -> None: + """Verify the first actor is persisted.""" + try: + context.shared_session.commit() + except Exception: + pass + + from cleveragents.infrastructure.database.models import ActorModel + + session = context.uow.session_factory() + try: + actor = session.query(ActorModel).filter_by(name="test-actor").first() + assert actor is not None, "First actor should be persisted" + finally: + session.close() + + +@then("the second actor creation should fail") +def step_second_actor_fails(context: Any) -> None: + """Verify the second actor creation failed.""" + assert len(context.errors) > 0, "The second actor creation should have failed" + + +@then("the first plan should be persisted") +def step_first_plan_persisted(context: Any) -> None: + """Verify the first plan is persisted.""" + try: + context.shared_session.commit() + except Exception: + pass + + from cleveragents.infrastructure.database.models import PlanModel + + session = context.uow.session_factory() + try: + plan = session.query(PlanModel).filter_by(project_id=1).first() + assert plan is not None, "First plan should be persisted" + finally: + session.close() + + +@then("the second plan creation should fail") +def step_second_plan_fails(context: Any) -> None: + """Verify the second plan creation failed.""" + assert len(context.errors) > 0, "The second plan creation should have failed" diff --git a/features/tdd_data_integrity_session_rollback_7489.feature b/features/tdd_data_integrity_session_rollback_7489.feature new file mode 100644 index 000000000..79ec9cea2 --- /dev/null +++ b/features/tdd_data_integrity_session_rollback_7489.feature @@ -0,0 +1,86 @@ +Feature: Data Integrity - Session Rollback on Shared UoW Session + As a developer + I want repositories to NOT call rollback() on shared UnitOfWork sessions + So that database errors in one repository don't roll back all pending work from other repositories + + Background: + Given I have a clean in-memory SQLite database + And I have initialized the database schema + + Scenario: ProjectRepository does not rollback shared session on error + Given I have a UnitOfWork with a shared session + And I have a ProjectRepository using the shared session + And I have an ActionRepository using the session factory + When I create an action in the shared session + And I flush the action to the database + And I attempt to create a project with invalid data that causes a database error + Then the action should still be persisted in the database + And the database error should be raised + And the shared session should NOT be rolled back by the repository + + Scenario: Multiple repositories in same UoW transaction + Given I have a UnitOfWork with a shared session + And I have a ProjectRepository using the shared session + And I have a ContextRepository using the shared session + When I create a project in the shared session + And I flush the project to the database + And I add context items to the shared session + And I flush the context to the database + And I attempt to create another project with invalid data + Then the first project should be persisted + And the context items should be persisted + And the second project creation should fail + And the shared session should NOT be rolled back by the repository + + Scenario: UnitOfWork manages transaction lifecycle exclusively + Given I have a UnitOfWork with a shared session + And I have a ProjectRepository using the shared session + When I create a project in the shared session + And I flush the project to the database + And I attempt to create a project with invalid data + Then the UnitOfWork should handle the rollback + And the repository should NOT call rollback() on the shared session + And the error should be propagated to the caller + + Scenario: Session-factory repositories can still rollback their own sessions + Given I have an ActionRepository using a session factory + When I create an action using the session factory + And I flush the action to the database + And I attempt to create a duplicate action + Then the ActionRepository should rollback its own session + And the duplicate action should not be persisted + And a DuplicateActionError should be raised + + Scenario: ChangeRepository does not rollback shared session + Given I have a UnitOfWork with a shared session + And I have a ChangeRepository using the shared session + And I have a DebugAttemptRepository using the shared session + When I add a change to the shared session + And I flush the change to the database + And I add a debug attempt to the shared session + And I flush the debug attempt to the database + And I attempt to add another change with invalid data + Then the first change should be persisted + And the debug attempt should be persisted + And the second change creation should fail + And the shared session should NOT be rolled back by the repository + + Scenario: ActorRepository does not rollback shared session + Given I have a UnitOfWork with a shared session + And I have an ActorRepository using the shared session + When I upsert an actor in the shared session + And I flush the actor to the database + And I attempt to upsert another actor with invalid data + Then the first actor should be persisted + And the second actor creation should fail + And the shared session should NOT be rolled back by the repository + + Scenario: PlanRepository does not rollback shared session + Given I have a UnitOfWork with a shared session + And I have a PlanRepository using the shared session + When I create a plan in the shared session + And I flush the plan to the database + And I attempt to create a plan with invalid data + Then the first plan should be persisted + And the second plan creation should fail + And the shared session should NOT be rolled back by the repository diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index ce10d80d1..d1a772a2f 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -173,7 +173,9 @@ class ProjectRepository: project.id = db_project.id # type: ignore return project except (OperationalError, SQLAlchemyDatabaseError) as e: - self.session.rollback() + # Do NOT call self.session.rollback() — the UnitOfWork owns this session + # and manages the transaction lifecycle. Calling rollback() here would + # roll back all pending work from other repositories in the same UoW. raise DatabaseError(f"Failed to create project: {e}") from e @database_retry