From a5888a08b718275bf9e67aae7df555a54941b8e5 Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Mon, 23 Mar 2026 06:21:37 +0000 Subject: [PATCH 1/2] =?UTF-8?q?test:=20add=20TDD=20bug-capture=20test=20fo?= =?UTF-8?q?r=20#987=20=E2=80=94=20AutomationProfileRepository=20session=20?= =?UTF-8?q?leak?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a Behave feature with four scenarios that capture the session leak bug in AutomationProfileRepository. The upsert() and delete() methods commit when auto_commit is True but never call session.close() in a finally block, leaking database sessions. SessionRepository correctly uses finally: if self._auto_commit: db_session.close() in every method — AutomationProfileRepository does not. The test uses a _TrackingSession subclass of sqlalchemy.orm.Session that records whether close() was called, providing direct assertion of the bug. Four scenarios cover: (1) upsert success path, (2) delete success path, (3) upsert error path, (4) delete error path. All scenarios are tagged @tdd_expected_fail @tdd_bug @tdd_bug_987 so CI passes while the bug remains unfixed. Fixes from review: - M1: Added 4th scenario for delete() error path - M2: Registered engine.dispose() in _cleanup_handlers for all scenarios - M3: Registered session.close() in _cleanup_handlers; explicitly close setup session in step_given_persisted_profile - M4: Replaced type: ignore annotations with proper subclass pattern (_TrackingSession uses **kwargs: Any; _FailingFlushTrackingSession overrides flush() instead of monkey-patching) - M5: Moved OperationalError import from function body to module level Robot integration tests are N/A — this is purely a unit-level session lifecycle bug within a single repository class. ISSUES CLOSED: #1092 --- CHANGELOG.md | 5 + ...d_automation_profile_session_leak_steps.py | 295 ++++++++++++++++++ ...dd_automation_profile_session_leak.feature | 43 +++ 3 files changed, 343 insertions(+) create mode 100644 features/steps/tdd_automation_profile_session_leak_steps.py create mode 100644 features/tdd_automation_profile_session_leak.feature diff --git a/CHANGELOG.md b/CHANGELOG.md index 352aa3d9a..ff6016f8d 100644 --- a/CHANGELOG.md +++ b/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`, diff --git a/features/steps/tdd_automation_profile_session_leak_steps.py b/features/steps/tdd_automation_profile_session_leak_steps.py new file mode 100644 index 000000000..ccfbeeeea --- /dev/null +++ b/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." + ) diff --git a/features/tdd_automation_profile_session_leak.feature b/features/tdd_automation_profile_session_leak.feature new file mode 100644 index 000000000..e48eac8df --- /dev/null +++ b/features/tdd_automation_profile_session_leak.feature @@ -0,0 +1,43 @@ +@tdd_expected_fail @tdd_bug @tdd_bug_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 -- 2.52.0 From 69e2d1f179f6cf2c0c7a4263250b30960dae5a74 Mon Sep 17 00:00:00 2001 From: "Brent E. Edwards" Date: Fri, 27 Mar 2026 22:00:57 +0000 Subject: [PATCH 2/2] test(tdd): add required issue tags for expected-fail session leak test Use @tdd_issue and @tdd_issue_987 so the TDD tag validation hook accepts this expected-fail bug-capture feature and unit tests can execute normally. --- features/tdd_automation_profile_session_leak.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/features/tdd_automation_profile_session_leak.feature b/features/tdd_automation_profile_session_leak.feature index e48eac8df..fcc1383d9 100644 --- a/features/tdd_automation_profile_session_leak.feature +++ b/features/tdd_automation_profile_session_leak.feature @@ -1,4 +1,4 @@ -@tdd_expected_fail @tdd_bug @tdd_bug_987 +@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 -- 2.52.0