diff --git a/CHANGELOG.md b/CHANGELOG.md index ba49ff3cc..613a3bf83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,19 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). versions (<3.13.4) cannot be installed even if upstream transitive dependencies have loose version constraints. +### Fixed + +- **Data integrity: removed session.rollback() calls from repository methods using shared sessions** (#7489): + Repository methods that receive a session from a shared `UnitOfWork` (via constructor injection + or session factory) were incorrectly calling `session.rollback()` when catching database errors. + This caused silent data loss: rolling back a shared session discards all pending writes from + other repositories in the same UnitOfWork transaction. Fixed by removing `session.rollback()` + calls from `ProjectRepository.create()`, `ActionRepository.create/update/delete()`, + `LifecyclePlanRepository.create/update/delete()`, and `ResourceRepository.create/update/delete/link_child/unlink_child/auto_discover_children()`. + The UnitOfWork is now the sole owner of the session and is responsible for transaction lifecycle + management (commit/rollback). Also replaced `# type: ignore` suppression in + `ProjectRepository.create()` with a proper `cast(int, ...)` call. + ### Changed - Restored `benchmark-regression` CI job to `master.yml` with `pull_request` trigger guard diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 8fd49fa4c..1ccb4ffa1 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -26,3 +26,4 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed automated bug fixes, CLI output formatting improvements, and ongoing maintenance as part of the CleverAgents automation system. * HAL 9000 has contributed the file edit encoding parameter fix (PR #8258 / issue #7559). * HAL 9000 has contributed comprehensive milestone documentation for v3.6.0 (Advanced Concepts & Deferred Features) and v3.7.0 (TUI Implementation) (PR #9903): split into sub-documents covering context strategies, LLM backends, resource types, A2A rename, container tool execution, scope chain resolution, cost/safety budgets, E2E workflow tests, code review examples, plugin architecture, TUI layout, persona system, reference/command input, session management, configuration, and TuiMaterializer integration. +* HAL 9000 has contributed the data integrity fix for issue #7489 (PR #8179): removed incorrect `session.rollback()` calls from repository methods that receive sessions from a shared UnitOfWork, preventing silent data loss when multiple repositories participate in the same transaction. 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..af663a5b7 --- /dev/null +++ b/features/steps/tdd_data_integrity_session_rollback_7489_steps.py @@ -0,0 +1,330 @@ +"""Step definitions for TDD issue #7489: repository methods must not call +session.rollback() on externally-owned sessions. + +These tests verify that the fix for issue #7489 is in place: repository +methods that receive a session from a UnitOfWork (via constructor injection +or session factory) must NOT call session.rollback() when catching database +errors, because the UoW owns the session and is responsible for rollback. +""" + +from __future__ import annotations + +from datetime import UTC, datetime +from types import SimpleNamespace +from typing import Any +from unittest.mock import MagicMock + +from behave import given, then, when +from behave.runner import Context +from sqlalchemy.exc import IntegrityError, OperationalError + +from cleveragents.core.exceptions import DatabaseError +from cleveragents.infrastructure.database.repositories import ( + ActionRepository, + DuplicateActionError, + DuplicatePlanError, + LifecyclePlanRepository, + ProjectRepository, + ResourceRepository, +) + + +# --------------------------------------------------------------------------- +# Shared mock session setup +# --------------------------------------------------------------------------- + + +@given("a shared mock session that tracks rollback calls") +def step_shared_mock_session(context: Context) -> None: + """Create a shared mock session that tracks rollback calls.""" + mock = MagicMock() + mock.rollback.return_value = None + mock.add.return_value = None + mock.flush.return_value = None + mock.refresh.return_value = None + mock.commit.return_value = None + mock.close.return_value = None + # Default query returns empty result + query_mock = MagicMock() + query_mock.filter_by.return_value.first.return_value = None + query_mock.filter.return_value.first.return_value = None + mock.query.return_value = query_mock + context.shared_session = mock + context.repo_error = None + + +@given("the shared session raises OperationalError on flush") +def step_session_raises_op_error_on_flush(context: Context) -> None: + """Configure the shared session to raise OperationalError on flush.""" + context.shared_session.flush.side_effect = OperationalError( + "disk I/O error", {}, None + ) + + +@given("the shared session raises IntegrityError on flush") +def step_session_raises_integrity_error_on_flush(context: Context) -> None: + """Configure the shared session to raise IntegrityError on flush.""" + context.shared_session.flush.side_effect = IntegrityError( + "UNIQUE constraint failed: table.name", {}, None + ) + + +# --------------------------------------------------------------------------- +# Repository setup steps +# --------------------------------------------------------------------------- + + +@given("a ProjectRepository using the shared session") +def step_project_repo_with_shared_session(context: Context) -> None: + """Create a ProjectRepository with the shared session.""" + context.repo_under_test = ProjectRepository(session=context.shared_session) + + +@given("an ActionRepository using the shared session factory") +def step_action_repo_with_shared_session(context: Context) -> None: + """Create an ActionRepository with a factory returning the shared session.""" + context.repo_under_test = ActionRepository( + session_factory=lambda: context.shared_session + ) + + +@given("a LifecyclePlanRepository using the shared session factory") +def step_plan_repo_with_shared_session(context: Context) -> None: + """Create a LifecyclePlanRepository with a factory returning the shared session.""" + context.repo_under_test = LifecyclePlanRepository( + session_factory=lambda: context.shared_session + ) + + +@given("a ResourceRepository using the shared session factory") +def step_resource_repo_with_shared_session(context: Context) -> None: + """Create a ResourceRepository with a factory returning the shared session.""" + context.repo_under_test = ResourceRepository( + session_factory=lambda: context.shared_session + ) + + +# --------------------------------------------------------------------------- +# Action steps +# --------------------------------------------------------------------------- + + +def _make_fake_project() -> Any: + """Create a minimal fake Project domain object.""" + from pathlib import Path + + from cleveragents.domain.models.core import Project, ProjectSettings + + return Project( + name="test-project", + path=Path("/tmp/test"), + settings=ProjectSettings(), + ) + + +def _make_fake_action() -> Any: + """Create a minimal fake Action domain object.""" + now = datetime.now(tz=UTC) + return SimpleNamespace( + namespaced_name=type( + "_NS", + (), + { + "namespace": "local", + "name": "test-action", + "__str__": lambda s: "local/test-action", + }, + )(), + description="A test action", + long_description=None, + definition_of_done="Done", + strategy_actor="local/strategist", + execution_actor="local/executor", + estimation_actor=None, + review_actor=None, + apply_actor=None, + invariant_actor=None, + automation_profile=None, + inputs_schema=None, + state=SimpleNamespace(value="available"), + reusable=False, + read_only=False, + created_by=None, + tags=[], + arguments=[], + invariants=[], + created_at=now, + updated_at=now, + ) + + +def _make_fake_plan() -> Any: + """Create a minimal fake Plan domain object.""" + now = datetime.now(tz=UTC) + return SimpleNamespace( + identity=SimpleNamespace( + plan_id="01HGZ6FE0A0000000000000099", + parent_plan_id=None, + root_plan_id=None, + attempt=1, + ), + phase=SimpleNamespace(value="strategize"), + processing_state=SimpleNamespace(value="queued"), + namespaced_name=type( + "_NS", (), {"namespace": "local", "__str__": lambda s: "local/test-plan"} + )(), + namespace="local", + action_name="local/test-action", + description="A test plan", + definition_of_done="Done", + strategy_actor="local/strategist", + execution_actor="local/executor", + review_actor=None, + apply_actor=None, + estimation_actor=None, + invariant_actor=None, + automation_profile=None, + error_message=None, + error_details=None, + changeset_id=None, + sandbox_refs=[], + validation_summary=None, + decision_root_id=None, + execution_environment=None, + execution_env_priority=None, + reusable=False, + read_only=False, + created_by=None, + tags=[], + project_links=[], + arguments={}, + arguments_order=[], + invariants=[], + effective_profile_snapshot="{}", + timestamps=SimpleNamespace( + created_at=now, + updated_at=now, + applied_at=None, + strategize_started_at=None, + strategize_completed_at=None, + execute_started_at=None, + execute_completed_at=None, + apply_started_at=None, + ), + reversion_count=0, + last_completed_step=None, + last_checkpoint_id=None, + cost_estimate_usd=None, + estimation_report=None, + ) + + +def _make_fake_resource() -> Any: + """Create a minimal fake Resource domain object.""" + return SimpleNamespace( + resource_id="01HGZ6FE0A0000000000000001", + name="test/my-resource", + resource_type_name="test/resource-type", + classification="physical", + description="A test resource", + location="/tmp/test", + capabilities=SimpleNamespace(writable=True), + sandbox_strategy=None, + content_hash=None, + properties=None, + ) + + +@when("I attempt to create a project via ProjectRepository") +def step_create_project(context: Context) -> None: + """Attempt to create a project, capturing any error.""" + try: + context.repo_under_test.create(_make_fake_project()) + except Exception as exc: + context.repo_error = exc + + +@when("I attempt to create an action via ActionRepository") +def step_create_action(context: Context) -> None: + """Attempt to create an action, capturing any error.""" + try: + context.repo_under_test.create(_make_fake_action()) + except Exception as exc: + context.repo_error = exc + + +@when("I attempt to create a plan via LifecyclePlanRepository") +def step_create_plan(context: Context) -> None: + """Attempt to create a plan, capturing any error.""" + try: + context.repo_under_test.create(_make_fake_plan()) + except Exception as exc: + context.repo_error = exc + + +@when("I attempt to create a resource via ResourceRepository") +def step_create_resource(context: Context) -> None: + """Attempt to create a resource, capturing any error.""" + # Configure the session to return a resource type row (so type validation passes) + type_row_mock = MagicMock() + type_row_mock.name = "test/resource-type" + query_mock = MagicMock() + # First query (for resource type) returns the type row + # Second query (for existing resource) returns None + query_mock.filter_by.return_value.first.side_effect = [type_row_mock, None] + context.shared_session.query.return_value = query_mock + try: + context.repo_under_test.create(_make_fake_resource()) + except Exception as exc: + context.repo_error = exc + + +# --------------------------------------------------------------------------- +# Assertion steps +# --------------------------------------------------------------------------- + + +@then("a DatabaseError should be raised") +def step_check_database_error(context: Context) -> None: + """Verify that a DatabaseError was raised.""" + assert context.repo_error is not None, ( + "Expected a DatabaseError but no error was raised" + ) + assert isinstance(context.repo_error, DatabaseError), ( + f"Expected DatabaseError, got {type(context.repo_error).__name__}: " + f"{context.repo_error}" + ) + + +@then("a DatabaseError or DuplicateActionError should be raised") +def step_check_database_or_duplicate_action_error(context: Context) -> None: + """Verify that a DatabaseError or DuplicateActionError was raised.""" + assert context.repo_error is not None, ( + "Expected a DatabaseError or DuplicateActionError but no error was raised" + ) + assert isinstance(context.repo_error, (DatabaseError, DuplicateActionError)), ( + f"Expected DatabaseError or DuplicateActionError, " + f"got {type(context.repo_error).__name__}: {context.repo_error}" + ) + + +@then("a DatabaseError or DuplicatePlanError should be raised") +def step_check_database_or_duplicate_plan_error(context: Context) -> None: + """Verify that a DatabaseError or DuplicatePlanError was raised.""" + assert context.repo_error is not None, ( + "Expected a DatabaseError or DuplicatePlanError but no error was raised" + ) + assert isinstance(context.repo_error, (DatabaseError, DuplicatePlanError)), ( + f"Expected DatabaseError or DuplicatePlanError, " + f"got {type(context.repo_error).__name__}: {context.repo_error}" + ) + + +@then("the shared session rollback should NOT have been called") +def step_check_no_rollback(context: Context) -> None: + """Verify that session.rollback() was NOT called on the shared session.""" + assert not context.shared_session.rollback.called, ( + f"Expected session.rollback() to NOT be called, but it was called " + f"{context.shared_session.rollback.call_count} time(s). " + f"Calls: {context.shared_session.rollback.call_args_list}" + ) 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..5b8e3492e --- /dev/null +++ b/features/tdd_data_integrity_session_rollback_7489.feature @@ -0,0 +1,56 @@ +@tdd_issue @tdd_issue_7489 +Feature: Repository methods do not call session.rollback() on externally-owned sessions + As a system using the UnitOfWork pattern for transaction management + I want repository methods to NOT call session.rollback() on shared sessions + So that previously flushed writes from other repositories are not silently discarded + + Background: + Given a shared mock session that tracks rollback calls + + @tdd_issue @tdd_issue_7489 + Scenario: ProjectRepository.create() does not rollback the shared session on OperationalError + Given a ProjectRepository using the shared session + And the shared session raises OperationalError on flush + When I attempt to create a project via ProjectRepository + Then a DatabaseError should be raised + And the shared session rollback should NOT have been called + + @tdd_issue @tdd_issue_7489 + Scenario: ActionRepository.create() does not rollback the shared session on OperationalError + Given an ActionRepository using the shared session factory + And the shared session raises OperationalError on flush + When I attempt to create an action via ActionRepository + Then a DatabaseError should be raised + And the shared session rollback should NOT have been called + + @tdd_issue @tdd_issue_7489 + Scenario: LifecyclePlanRepository.create() does not rollback the shared session on OperationalError + Given a LifecyclePlanRepository using the shared session factory + And the shared session raises OperationalError on flush + When I attempt to create a plan via LifecyclePlanRepository + Then a DatabaseError should be raised + And the shared session rollback should NOT have been called + + @tdd_issue @tdd_issue_7489 + Scenario: ResourceRepository.create() does not rollback the shared session on OperationalError + Given a ResourceRepository using the shared session factory + And the shared session raises OperationalError on flush + When I attempt to create a resource via ResourceRepository + Then a DatabaseError should be raised + And the shared session rollback should NOT have been called + + @tdd_issue @tdd_issue_7489 + Scenario: ActionRepository.create() does not rollback the shared session on IntegrityError + Given an ActionRepository using the shared session factory + And the shared session raises IntegrityError on flush + When I attempt to create an action via ActionRepository + Then a DatabaseError or DuplicateActionError should be raised + And the shared session rollback should NOT have been called + + @tdd_issue @tdd_issue_7489 + Scenario: LifecyclePlanRepository.create() does not rollback the shared session on IntegrityError + Given a LifecyclePlanRepository using the shared session factory + And the shared session raises IntegrityError on flush + When I attempt to create a plan via LifecyclePlanRepository + Then a DatabaseError or DuplicatePlanError should be raised + And the shared session rollback should NOT have been called diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 9a882377d..c7c3e233a 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -171,7 +171,7 @@ class ProjectRepository: self.session.flush() self.session.refresh(db_project) - project.id = db_project.id # type: ignore + project.id = cast(int, db_project.id) return project except (OperationalError, SQLAlchemyDatabaseError) as e: raise DatabaseError(f"Failed to create project: {e}") from e @@ -994,13 +994,13 @@ class ActionRepository(ActionRepositoryProtocol): session.flush() return action except IntegrityError as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session # Unique constraint on ``name`` column if "UNIQUE" in str(exc).upper() or "unique" in str(exc).lower(): raise DuplicateActionError(str(action.namespaced_name)) from exc raise DatabaseError(f"Failed to create action: {exc}") from exc except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError(f"Failed to create action: {exc}") from exc @database_retry @@ -1242,7 +1242,7 @@ class ActionRepository(ActionRepositoryProtocol): session.flush() return action except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError( f"Failed to update action {action_name_str}: {exc}" ) from exc @@ -1317,7 +1317,7 @@ class ActionRepository(ActionRepositoryProtocol): except ActionInUseError: raise except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError( f"Failed to delete action {action_name}: {exc}" ) from exc @@ -1387,13 +1387,13 @@ class LifecyclePlanRepository(LifecyclePlanRepositoryProtocol): session.flush() return plan except IntegrityError as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session exc_str = str(exc).upper() if "UNIQUE" in exc_str or "PRIMARY" in exc_str: raise DuplicatePlanError(str(plan.identity.plan_id)) from exc raise DatabaseError(f"Failed to create plan: {exc}") from exc except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError(f"Failed to create plan: {exc}") from exc @database_retry @@ -1603,7 +1603,7 @@ class LifecyclePlanRepository(LifecyclePlanRepositoryProtocol): except PlanNotFoundError: raise except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError(f"Failed to update plan {plan_id_str}: {exc}") from exc @database_retry @@ -1647,7 +1647,7 @@ class LifecyclePlanRepository(LifecyclePlanRepositoryProtocol): session.flush() return True except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError(f"Failed to delete plan {plan_id}: {exc}") from exc @database_retry @@ -2259,14 +2259,14 @@ class ResourceRepository: except (ResourceTypeNotFoundError, DuplicateResourceError): raise except IntegrityError as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session if "UNIQUE" in str(exc).upper() or "unique" in str(exc).lower(): raise DuplicateResourceError( resource.name or resource.resource_id ) from exc raise DatabaseError(f"Failed to create resource: {exc}") from exc except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError(f"Failed to create resource: {exc}") from exc @database_retry @@ -2395,7 +2395,7 @@ class ResourceRepository: except ResourceNotFoundRepoError: raise except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError( f"Failed to update resource '{resource.resource_id}': {exc}" ) from exc @@ -2440,7 +2440,7 @@ class ResourceRepository: except ResourceHasEdgesError: raise except (OperationalError, SQLAlchemyDatabaseError) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError( f"Failed to delete resource '{resource_id}': {exc}" ) from exc @@ -2532,7 +2532,7 @@ class ResourceRepository: ): raise except IntegrityError as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError( f"Failed to link {parent_id} -> {child_id}: {exc}" ) from exc @@ -2540,7 +2540,7 @@ class ResourceRepository: OperationalError, SQLAlchemyDatabaseError, ) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError( f"Failed to link {parent_id} -> {child_id}: {exc}" ) from exc @@ -2591,7 +2591,7 @@ class ResourceRepository: OperationalError, SQLAlchemyDatabaseError, ) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError( f"Failed to unlink {parent_id} -> {child_id}: {exc}" ) from exc @@ -2802,7 +2802,7 @@ class ResourceRepository: OperationalError, SQLAlchemyDatabaseError, ) as exc: - session.rollback() + # Do NOT call session.rollback() — the UoW owns this session raise DatabaseError( f"Failed to auto-discover children for '{resource_id}': {exc}" ) from exc