CHANGELOG.md

@@ -130,6 +130,11 @@
   with `shlex.split()` and `shell=False` for defense-in-depth command injection
   prevention, consistent with the existing pattern in
   `cli_plan_context_commands_steps.py`. (#734)
+- Added TDD bug-capture test for bug #987: AutomationProfileRepository session
+  leak. Four Behave BDD scenarios verify that `upsert()` and `delete()` close
+  the database session in `auto_commit` mode, capturing the missing
+  `session.close()` in a `finally` block. Tests use `@tdd_expected_fail` until
+  the bug fix is merged. (#1092)
 - Added ACMS Backend Abstraction Layer (BAL) protocol definitions and
   in-memory stub implementations. Defines `TextBackend`, `VectorBackend`,
   and `GraphBackend` protocols with frozen result dataclasses (`TextResult`,

features/steps/tdd_automation_profile_session_leak_steps.py

@@ -0,0 +1,295 @@
+"""Step definitions for TDD Bug #987 — AutomationProfileRepository session leak.
+
+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.
+"""
+
+from __future__ import annotations
+
+from collections.abc import Sequence
+from typing import Any
+
+from behave import given, then, when
+from behave.runner import Context
+from sqlalchemy import create_engine
+from sqlalchemy.exc import OperationalError
+from sqlalchemy.orm import Session, sessionmaker
+
+from cleveragents.core.exceptions import DatabaseError
+from cleveragents.domain.models.core.automation_profile import AutomationProfile
+from cleveragents.infrastructure.database.models import Base
+from cleveragents.infrastructure.database.repositories import (
+    AutomationProfileRepository,
+)
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def _make_profile(name: str = "local/leak-test-profile") -> AutomationProfile:
+    """Create a minimal valid ``AutomationProfile`` for testing."""
+    return AutomationProfile(
+        name=name,
+        description="TDD test profile for session leak bug #987",
+    )
+
+
+class _TrackingSession(Session):
+    """A thin ``Session`` subclass that records whether ``close()`` was called.
+
+    Uses keyword-only ``bind`` parameter to match
+    ``sqlalchemy.orm.Session.__init__`` without requiring ``type: ignore``.
+    """
+
+    close_called: bool
+
+    def __init__(self, **kwargs: Any) -> None:
+        super().__init__(**kwargs)
+        self.close_called = False
+
+    def close(self) -> None:
+        self.close_called = True
+        super().close()
+
+
+class _FailingFlushTrackingSession(_TrackingSession):
+    """A ``_TrackingSession`` whose ``flush()`` always raises.
+
+    Used to test the error path without monkey-patching, eliminating the
+    need for a ``type: ignore[assignment]`` annotation.
+    """
+
+    def flush(self, objects: Sequence[Any] | None = None) -> None:
+        raise OperationalError("simulated failure", params=None, orig=Exception("boom"))
+
+
+# ---------------------------------------------------------------------------
+# Given steps
+# ---------------------------------------------------------------------------
+
+
+@given(
+    "an AutomationProfileRepository with auto_commit enabled"
+    " and a tracking session factory"
+)
+def step_given_repo_with_tracking_factory(context: Context) -> None:
+    """Set up a real in-memory SQLite database with a tracking session."""
+    engine = create_engine("sqlite:///:memory:", echo=False)
+    Base.metadata.create_all(engine)
+
+    # We need to keep a reference to the tracking session so we can
+    # inspect ``close_called`` later.
+    tracking_session = _TrackingSession(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
+    # so resources are released even if the scenario fails mid-way.
+    if not hasattr(context, "_cleanup_handlers"):
+        context._cleanup_handlers = []
+    context._cleanup_handlers.append(tracking_session.close)
+    context._cleanup_handlers.append(engine.dispose)
+
+
+@given('a persisted automation profile named "{name}"')
+def step_given_persisted_profile(context: Context, name: str) -> None:
+    """Pre-populate a profile so that delete can find it."""
+    profile = _make_profile(name)
+    # Use a fresh session to insert the profile directly, bypassing
+    # the repository under test so we don't conflate setup with the
+    # action being tested.
+    fresh_factory = sessionmaker(bind=context.db_engine)
+    setup_session = fresh_factory()
+    setup_repo = AutomationProfileRepository(
+        session_factory=lambda: setup_session,
+        auto_commit=True,
+    )
+    setup_repo.upsert(profile)
+    # Explicitly close the setup session to avoid leaking it.
+    setup_session.close()
+
+    # Reset the tracking session's close flag so the test only
+    # measures the ``delete()`` call.
+    context.tracking_session.close_called = False
+
+
+@given(
+    "an AutomationProfileRepository with auto_commit enabled"
+    " and a failing session factory"
+)
+def step_given_repo_with_failing_factory(context: Context) -> None:
+    """Set up a repository whose session will raise on flush."""
+    engine = create_engine("sqlite:///:memory:", echo=False)
+    Base.metadata.create_all(engine)
+
+    tracking_session = _FailingFlushTrackingSession(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)
+
+
+@given(
+    "an AutomationProfileRepository with auto_commit enabled,"
+    " a pre-populated profile,"
+    " and a failing-flush tracking session"
+)
+def step_given_repo_with_prepopulated_and_failing(context: Context) -> None:
+    """Set up a repo with existing data and a session that fails on flush.
+
+    This exercises the ``delete()`` error path: the query succeeds (finds
+    the profile) but the subsequent ``flush()`` after ``session.delete()``
+    raises an ``OperationalError``.
+    """
+    engine = create_engine("sqlite:///:memory:", echo=False)
+    Base.metadata.create_all(engine)
+
+    # Pre-populate a profile using a normal session.
+    setup_factory = sessionmaker(bind=engine)
+    setup_session = setup_factory()
+    setup_repo = AutomationProfileRepository(
+        session_factory=lambda: setup_session,
+        auto_commit=True,
+    )
+    setup_repo.upsert(_make_profile())
+    setup_session.close()
+
+    # Now create the failing-flush tracking session for the test.
+    tracking_session = _FailingFlushTrackingSession(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
+# ---------------------------------------------------------------------------
+
+
+@when("I upsert a valid automation profile via the repository")
+def step_when_upsert_profile(context: Context) -> None:
+    """Call upsert on the repository under test."""
+    profile = _make_profile()
+    context.repo.upsert(profile)
+
+
+@when('I delete the automation profile "{name}" via the repository')
+def step_when_delete_profile(context: Context, name: str) -> None:
+    """Call delete on the repository under test."""
+    context.repo.delete(name)
+
+
+@when("I attempt to upsert a profile that triggers a database error")
+def step_when_upsert_triggers_error(context: Context) -> None:
+    """Attempt an upsert that will fail due to the failing flush."""
+    profile = _make_profile("local/error-profile")
+    context.upsert_error = None
+    try:
+        context.repo.upsert(profile)
+    except DatabaseError as exc:
+        context.upsert_error = exc
+
+
+@when("I attempt to delete a profile that triggers a database error")
+def step_when_delete_triggers_error(context: Context) -> None:
+    """Attempt a delete that will fail due to the failing flush."""
+    context.delete_error = None
+    try:
+        context.repo.delete("local/leak-test-profile")
+    except DatabaseError as exc:
+        context.delete_error = exc
+
+
+# ---------------------------------------------------------------------------
+# Then steps
+# ---------------------------------------------------------------------------
+
+
+@then("the tracking session should have been closed")
+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.
+    """
+    assert context.tracking_session.close_called, (
+        "Expected session.close() to have been called in auto_commit mode, "
+        "but it was NOT called. This confirms bug #987: "
+        "AutomationProfileRepository leaks sessions."
+    )
+
+
+@then("the tracking session should have been closed despite the error")
+def step_then_session_closed_despite_error(context: Context) -> None:
+    """Assert close() was called even when an error occurred.
+
+    The finally block should ensure session cleanup regardless of
+    whether the operation succeeded or failed.
+    """
+    assert context.upsert_error is not None, (
+        "Expected a DatabaseError from the failing upsert, 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 delete error")
+def step_then_session_closed_despite_delete_error(context: Context) -> None:
+    """Assert close() was called even when a delete error occurred.
+
+    The finally block should ensure session cleanup regardless of
+    whether the operation succeeded or failed.
+    """
+    assert context.delete_error is not None, (
+        "Expected a DatabaseError from the failing delete, 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."
+    )

features/tdd_automation_profile_session_leak.feature

@@ -0,0 +1,43 @@
+@tdd_expected_fail @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
+
+  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.
+
+  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.
+
+  Scenario: upsert closes session in auto_commit mode on success
+    Given an AutomationProfileRepository with auto_commit enabled and a tracking session factory
+    When I upsert a valid automation profile via the repository
+    Then the tracking session should have been closed
+
+  Scenario: delete closes session in auto_commit mode on success
+    Given an AutomationProfileRepository with auto_commit enabled and a tracking session factory
+    And a persisted automation profile named "local/leak-test-profile"
+    When I delete the automation profile "local/leak-test-profile" via the repository
+    Then the tracking session should have been closed
+
+  Scenario: upsert closes session in auto_commit mode on database error
+    Given an AutomationProfileRepository with auto_commit enabled and a failing session factory
+    When I attempt to upsert a profile that triggers a database error
+    Then the tracking session should have been closed despite the error
+
+  Scenario: delete closes session in auto_commit mode on database error
+    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