diff --git a/CHANGELOG.md b/CHANGELOG.md index 74fde1e4..18198c04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,12 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- Fixed session leak in all `AutomationProfileRepository` public methods + (`get_by_name()`, `list_all()`, `upsert()`, and `delete()`): added + `finally: if self._auto_commit: session.close()` blocks matching the + pattern already used by `SessionRepository`, preventing database sessions + from being leaked when `auto_commit` mode is enabled. (#987) + - **CLI — `agents actor add` rich output**: The `actor add` command now renders the full spec-required output including **Type**, **Config**, **Capabilities**, and **Tools** panels, matching the output format of `actor show`. diff --git a/features/steps/tdd_automation_profile_session_leak_steps.py b/features/steps/tdd_automation_profile_session_leak_steps.py index ccfbeeee..74bdaa91 100644 --- a/features/steps/tdd_automation_profile_session_leak_steps.py +++ b/features/steps/tdd_automation_profile_session_leak_steps.py @@ -1,14 +1,10 @@ -"""Step definitions for TDD Bug #987 — AutomationProfileRepository session leak. +"""Step definitions for bug #987 regression coverage. -This test captures bug #987: ``AutomationProfileRepository.upsert()`` and -``delete()`` never call ``session.close()`` when using ``auto_commit`` mode. -By contrast, ``SessionRepository`` correctly uses -``finally: if self._auto_commit: db_session.close()`` in every method. - -The assertions here will **fail** until the bug is fixed, proving the bug -exists. The ``@tdd_expected_fail`` tag inverts the result so CI passes. - -This test uses ``@tdd_expected_fail`` until the fix for #987 is merged. +These scenarios verify the fixed behavior for all public +``AutomationProfileRepository`` methods (``get_by_name()``, +``list_all()``, ``upsert()``, and ``delete()``): when ``auto_commit`` +mode is enabled, each method must close its session in a ``finally`` +block, matching the existing ``SessionRepository`` pattern. """ from __future__ import annotations @@ -71,6 +67,21 @@ class _FailingFlushTrackingSession(_TrackingSession): raise OperationalError("simulated failure", params=None, orig=Exception("boom")) +class _FailingQueryTrackingSession(_TrackingSession): + """A ``_TrackingSession`` whose ``query()`` always raises. + + Used to test the error path for read-only methods (``get_by_name``, + ``list_all``) without monkey-patching. + """ + + def query(self, *entities: Any, **kwargs: Any) -> Any: + raise OperationalError( + "simulated query failure", + params=None, + orig=Exception("boom"), + ) + + # --------------------------------------------------------------------------- # Given steps # --------------------------------------------------------------------------- @@ -202,6 +213,34 @@ def step_given_repo_with_prepopulated_and_failing(context: Context) -> None: context._cleanup_handlers.append(engine.dispose) +@given( + "an AutomationProfileRepository with auto_commit enabled" + " and a failing-query session factory" +) +def step_given_repo_with_failing_query_factory(context: Context) -> None: + """Set up a repository whose session will raise on query.""" + engine = create_engine("sqlite:///:memory:", echo=False) + Base.metadata.create_all(engine) + + tracking_session = _FailingQueryTrackingSession(bind=engine) + context.tracking_session = tracking_session + context.db_engine = engine + + def session_factory() -> Session: + return tracking_session + + context.repo = AutomationProfileRepository( + session_factory=session_factory, + auto_commit=True, + ) + + # Register cleanup handlers for engine disposal and session close. + if not hasattr(context, "_cleanup_handlers"): + context._cleanup_handlers = [] + context._cleanup_handlers.append(tracking_session.close) + context._cleanup_handlers.append(engine.dispose) + + # --------------------------------------------------------------------------- # When steps # --------------------------------------------------------------------------- @@ -241,6 +280,38 @@ def step_when_delete_triggers_error(context: Context) -> None: context.delete_error = exc +@when("I look up a profile by name via the repository") +def step_when_get_by_name(context: Context) -> None: + """Call get_by_name on the repository under test.""" + context.repo.get_by_name("local/nonexistent-profile") + + +@when("I list all profiles via the repository") +def step_when_list_all(context: Context) -> None: + """Call list_all on the repository under test.""" + context.repo.list_all() + + +@when("I attempt to look up a profile that triggers a database error") +def step_when_get_by_name_triggers_error(context: Context) -> None: + """Attempt a get_by_name that will fail due to the failing query.""" + context.get_by_name_error = None + try: + context.repo.get_by_name("local/nonexistent-profile") + except DatabaseError as exc: + context.get_by_name_error = exc + + +@when("I attempt to list profiles that triggers a database error") +def step_when_list_all_triggers_error(context: Context) -> None: + """Attempt a list_all that will fail due to the failing query.""" + context.list_all_error = None + try: + context.repo.list_all() + except DatabaseError as exc: + context.list_all_error = exc + + # --------------------------------------------------------------------------- # Then steps # --------------------------------------------------------------------------- @@ -250,9 +321,9 @@ def step_when_delete_triggers_error(context: Context) -> None: def step_then_session_closed(context: Context) -> None: """Assert that close() was called on the tracking session. - This assertion will FAIL on the current codebase because - AutomationProfileRepository does not call session.close() in a - finally block when auto_commit is True — proving bug #987. + Regression guard for bug #987: verifies that + AutomationProfileRepository calls session.close() in a finally + block when auto_commit is True. """ assert context.tracking_session.close_called, ( "Expected session.close() to have been called in auto_commit mode, " @@ -293,3 +364,37 @@ def step_then_session_closed_despite_delete_error(context: Context) -> None: "error in auto_commit mode, but it was NOT called. This confirms " "bug #987: AutomationProfileRepository leaks sessions on error." ) + + +@then("the tracking session should have been closed despite the get_by_name error") +def step_then_session_closed_despite_get_by_name_error(context: Context) -> None: + """Assert close() was called even when a get_by_name error occurred. + + The finally block should ensure session cleanup regardless of + whether the query succeeded or failed. + """ + assert context.get_by_name_error is not None, ( + "Expected a DatabaseError from the failing get_by_name, but none was raised." + ) + assert context.tracking_session.close_called, ( + "Expected session.close() to have been called even after a database " + "error in auto_commit mode, but it was NOT called. This confirms " + "bug #987: AutomationProfileRepository leaks sessions on error." + ) + + +@then("the tracking session should have been closed despite the list_all error") +def step_then_session_closed_despite_list_all_error(context: Context) -> None: + """Assert close() was called even when a list_all error occurred. + + The finally block should ensure session cleanup regardless of + whether the query succeeded or failed. + """ + assert context.list_all_error is not None, ( + "Expected a DatabaseError from the failing list_all, but none was raised." + ) + assert context.tracking_session.close_called, ( + "Expected session.close() to have been called even after a database " + "error in auto_commit mode, but it was NOT called. This confirms " + "bug #987: AutomationProfileRepository leaks sessions on error." + ) diff --git a/features/tdd_automation_profile_session_leak.feature b/features/tdd_automation_profile_session_leak.feature index fcc1383d..0724d06a 100644 --- a/features/tdd_automation_profile_session_leak.feature +++ b/features/tdd_automation_profile_session_leak.feature @@ -1,25 +1,20 @@ -@tdd_expected_fail @tdd_issue @tdd_issue_987 +@tdd_issue @tdd_issue_987 Feature: TDD Bug #987 — AutomationProfileRepository session leak As a developer I want to verify that AutomationProfileRepository closes sessions in auto_commit mode - So that the bug is captured and will be caught by a regression test + So that regressions in session cleanup are caught - AutomationProfileRepository.upsert() and delete() commit when - auto_commit is True but never call session.close() in a finally - block. By contrast, SessionRepository correctly uses - ``finally: if self._auto_commit: db_session.close()`` in every - method. + Regression guard for bug #987. AutomationProfileRepository methods + previously committed when auto_commit was True but never called + session.close() in a finally block. The fix added + ``finally: if self._auto_commit: session.close()`` to all public + session-creating methods (get_by_name, list_all, upsert, delete), + matching the pattern already used by SessionRepository. - This inconsistency means AutomationProfileRepository leaks database - sessions over time, potentially exhausting the connection pool. - - These tests assert the expected behaviour (session.close() IS called) - and will FAIL until the bug is fixed. The @tdd_expected_fail tag - inverts the result so CI passes. - - # This test captures bug #987 and uses @tdd_expected_fail until the - # fix is merged. + These scenarios verify that sessions are properly closed in + auto_commit mode for both success and error paths across all + public methods. Scenario: upsert closes session in auto_commit mode on success Given an AutomationProfileRepository with auto_commit enabled and a tracking session factory @@ -41,3 +36,23 @@ Feature: TDD Bug #987 — AutomationProfileRepository session leak Given an AutomationProfileRepository with auto_commit enabled, a pre-populated profile, and a failing-flush tracking session When I attempt to delete a profile that triggers a database error Then the tracking session should have been closed despite the delete error + + Scenario: get_by_name closes session in auto_commit mode on success + Given an AutomationProfileRepository with auto_commit enabled and a tracking session factory + When I look up a profile by name via the repository + Then the tracking session should have been closed + + Scenario: list_all closes session in auto_commit mode on success + Given an AutomationProfileRepository with auto_commit enabled and a tracking session factory + When I list all profiles via the repository + Then the tracking session should have been closed + + Scenario: get_by_name closes session in auto_commit mode on database error + Given an AutomationProfileRepository with auto_commit enabled and a failing-query session factory + When I attempt to look up a profile that triggers a database error + Then the tracking session should have been closed despite the get_by_name error + + Scenario: list_all closes session in auto_commit mode on database error + Given an AutomationProfileRepository with auto_commit enabled and a failing-query session factory + When I attempt to list profiles that triggers a database error + Then the tracking session should have been closed despite the list_all error diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index e869e197..29b366a0 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -4378,6 +4378,9 @@ class AutomationProfileRepository: SQLAlchemyDatabaseError, ) as exc: raise DatabaseError(f"Failed to get profile '{name}': {exc}") from exc + finally: + if self._auto_commit: + session.close() @database_retry def list_all(self) -> list[Any]: @@ -4399,6 +4402,9 @@ class AutomationProfileRepository: SQLAlchemyDatabaseError, ) as exc: raise DatabaseError(f"Failed to list profiles: {exc}") from exc + finally: + if self._auto_commit: + session.close() @database_retry def upsert( @@ -4460,6 +4466,9 @@ class AutomationProfileRepository: raise DatabaseError( f"Failed to upsert profile '{profile.name}': {exc}" ) from exc + finally: + if self._auto_commit: + session.close() @database_retry def delete(self, name: str) -> None: @@ -4489,6 +4498,9 @@ class AutomationProfileRepository: ) as exc: session.rollback() raise DatabaseError(f"Failed to delete profile '{name}': {exc}") from exc + finally: + if self._auto_commit: + session.close() @staticmethod def _to_domain(