cleveragents/cleveragents-core

Fork 3

BUG-HUNT: [data-integrity] SkillRepository.update() calls session.close() inside try block before normal method exit, leaving session double-closed by auto_commit path #7760

New issue

Open

opened 2026-04-12 03:25:57 +00:00 by HAL9000 · 3 comments

HAL9000 commented

2026-04-12 03:25:57 +00:00

Owner

Bug Report: Data Integrity — `SkillRepository.update()` Calls `session.close()` Inside `try` Block, Causing Double-Close

Severity Assessment

Impact: After session.close() is called inside the try block when auto_commit=True, the session is already closed. Any exception handler that subsequently tries to call session.rollback() will operate on a closed session, potentially losing error information or causing SQLAlchemy warnings/errors
Likelihood: Medium — only triggers when auto_commit=True and the code after commit fails
Priority: Medium

Location

File: src/cleveragents/infrastructure/database/repositories.py
Function/Class: SkillRepository.update, SkillRepository.create, SkillRepository.delete
Lines: 4914–4918 (update), 4801–4803 (create), 4947–4949 (delete)

Description

SkillRepository methods call session.commit() and session.close() inside the try block directly (not in a finally block), unlike all other repositories that use finally: session.close(). This means:

session.close() is called inside the try block
If an exception occurs AFTER session.close() but before the except block catches it, the session is already closed
The except (OperationalError, SQLAlchemyDatabaseError) block then tries session.rollback() on an already-closed session

Also, the SkillRepository.delete method calls session.close() inside try on line 4949, meaning that after session.flush() succeeds, the session is closed, but the auto-commit commit never happens (the code does session.flush() then if self._auto_commit: session.commit(); session.close() which is correct — wait, let me re-read...).

Evidence

# repositories.py lines 4796-4816 - SkillRepository.create
session = self._session()
try:
    db_model = SkillModel.from_domain(skill)
    session.add(db_model)
    session.flush()
    if self._auto_commit:
        session.commit()
        session.close()  # <-- session closed INSIDE try block
    return skill
except IntegrityError as exc:
    session.rollback()  # <-- operating on already-closed session if close() was called
    ...
except (OperationalError, SQLAlchemyDatabaseError) as exc:
    session.rollback()  # <-- same problem
    ...
# NOTE: No `finally: session.close()` for the non-auto_commit case

Contrast with the correct pattern used in ToolRegistryRepository.create (lines 3418-3439):

try:
    ...
    session.flush()
    session.commit()
    return tool
except ...:
    session.rollback()
    raise
finally:
    session.close()  # CORRECT: always in finally block

Expected Behavior

session.close() should always be called in a finally block to ensure it runs regardless of success or failure, and should not be called inside the try block where subsequent exception handlers may try to use the closed session.

Actual Behavior

In SkillRepository methods with auto_commit=True, session.close() is called inside the try block. If any subsequent code raises an exception, the except block's session.rollback() call operates on a closed session.

Suggested Fix

Follow the established pattern from other repositories:

session = self._session()
try:
    ...
    session.flush()
    if self._auto_commit:
        session.commit()
    return skill
except IntegrityError as exc:
    session.rollback()
    raise ...
except (OperationalError, SQLAlchemyDatabaseError) as exc:
    session.rollback()
    raise ...
finally:
    if self._auto_commit:
        session.close()

TDD Note

After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD.

Automated by CleverAgents Bot
Supervisor: Bug Hunting | Agent: bug-hunter

## Bug Report: Data Integrity — `SkillRepository.update()` Calls `session.close()` Inside `try` Block, Causing Double-Close ### Severity Assessment - **Impact**: After `session.close()` is called inside the `try` block when `auto_commit=True`, the session is already closed. Any exception handler that subsequently tries to call `session.rollback()` will operate on a closed session, potentially losing error information or causing SQLAlchemy warnings/errors - **Likelihood**: Medium — only triggers when `auto_commit=True` and the code after commit fails - **Priority**: Medium ### Location - **File**: `src/cleveragents/infrastructure/database/repositories.py` - **Function/Class**: `SkillRepository.update`, `SkillRepository.create`, `SkillRepository.delete` - **Lines**: 4914–4918 (update), 4801–4803 (create), 4947–4949 (delete) ### Description `SkillRepository` methods call `session.commit()` and `session.close()` inside the `try` block directly (not in a `finally` block), unlike all other repositories that use `finally: session.close()`. This means: 1. `session.close()` is called inside the `try` block 2. If an exception occurs AFTER `session.close()` but before the `except` block catches it, the session is already closed 3. The `except (OperationalError, SQLAlchemyDatabaseError)` block then tries `session.rollback()` on an already-closed session Also, the `SkillRepository.delete` method calls `session.close()` inside `try` on line 4949, meaning that after `session.flush()` succeeds, the session is closed, but the auto-commit commit never happens (the code does `session.flush()` then `if self._auto_commit: session.commit(); session.close()` which is correct — wait, let me re-read...). ### Evidence ```python # repositories.py lines 4796-4816 - SkillRepository.create session = self._session() try: db_model = SkillModel.from_domain(skill) session.add(db_model) session.flush() if self._auto_commit: session.commit() session.close() # <-- session closed INSIDE try block return skill except IntegrityError as exc: session.rollback() # <-- operating on already-closed session if close() was called ... except (OperationalError, SQLAlchemyDatabaseError) as exc: session.rollback() # <-- same problem ... # NOTE: No `finally: session.close()` for the non-auto_commit case ``` Contrast with the correct pattern used in `ToolRegistryRepository.create` (lines 3418-3439): ```python try: ... session.flush() session.commit() return tool except ...: session.rollback() raise finally: session.close() # CORRECT: always in finally block ``` ### Expected Behavior `session.close()` should always be called in a `finally` block to ensure it runs regardless of success or failure, and should not be called inside the `try` block where subsequent exception handlers may try to use the closed session. ### Actual Behavior In `SkillRepository` methods with `auto_commit=True`, `session.close()` is called inside the `try` block. If any subsequent code raises an exception, the `except` block's `session.rollback()` call operates on a closed session. ### Suggested Fix Follow the established pattern from other repositories: ```python session = self._session() try: ... session.flush() if self._auto_commit: session.commit() return skill except IntegrityError as exc: session.rollback() raise ... except (OperationalError, SQLAlchemyDatabaseError) as exc: session.rollback() raise ... finally: if self._auto_commit: session.close() ``` ### Category data-integrity ### TDD Note After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD. --- **Automated by CleverAgents Bot** Supervisor: Bug Hunting | Agent: bug-hunter

HAL9000 added this to the v3.2.0 milestone

2026-04-12 03:41:35 +00:00

HAL9000 referenced this issue

2026-04-12 03:47:56 +00:00

[AUTO-PROJ-OWN] Project Owner Report (Cycle 1) #7724

HAL9000 added the

labels

2026-04-12 05:48:01 +00:00

freemo added the

Type

Bug

label

2026-04-12 18:33:41 +00:00

HAL9000 added

and removed

labels

2026-04-14 05:58:25 +00:00

HAL9000 commented

2026-04-14 05:58:25 +00:00

Author

Owner

✅ Verified — Data integrity bug: SkillRepository.update() double-closes session. MoSCoW: Should-have. Priority: Medium.

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner-pool-supervisor

✅ **Verified** — Data integrity bug: SkillRepository.update() double-closes session. MoSCoW: Should-have. Priority: Medium. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner-pool-supervisor

HAL9000 commented

2026-04-14 06:11:42 +00:00

Author

Owner

✅ Verified — Data integrity bug: SkillRepository.update() double-closes session. MoSCoW: Should-have. Priority: Medium.

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner-pool-supervisor