cleveragents/cleveragents-core

Fork 3

BUG-HUNT: [concurrency] MEMORY_ENGINES global dict has TOCTOU race condition and inconsistent engine settings across callers #6453

New issue

Open

opened 2026-04-09 21:04:46 +00:00 by HAL9000 · 1 comment

HAL9000 commented

2026-04-09 21:04:46 +00:00

Owner

Bug Report: Concurrency — `MEMORY_ENGINES` global cache has race condition and creates engines with different settings

Severity Assessment

Impact: In multi-threaded usage (e.g. during test runs or async workers), two threads can simultaneously observe the sqlite:///:memory: key missing from MEMORY_ENGINES and each create a separate in-memory SQLite engine. The second thread's engine overwrites the first in the dict. The first thread now holds a reference to an engine that no other code knows about — schema migrations may run against a different engine than the one actually used. Additionally, the same key may be populated with engines having different configurations depending on which caller runs first.
Likelihood: Likely in parallel test execution (e.g. pytest-xdist) and possible in any multi-threaded process that uses in-memory SQLite. Silent corruption with no error raised.
Priority: Medium

Location

File 1: src/cleveragents/infrastructure/database/engine_cache.py
Function/Class: Module-level MEMORY_ENGINES dict
Lines: 15 (definition)
File 2: src/cleveragents/infrastructure/database/unit_of_work.py
Function/Class: UnitOfWork.engine property
Lines: 76–84
File 3: src/cleveragents/infrastructure/database/migration_runner.py
Function/Class: MigrationRunner.init_or_upgrade()
Lines: 248–253

Description

Issue A — TOCTOU Race Condition

Both unit_of_work.py and migration_runner.py use a check-then-set pattern to populate MEMORY_ENGINES:

# unit_of_work.py lines 76-84
if self.database_url not in MEMORY_ENGINES:        # ← CHECK
    MEMORY_ENGINES[self.database_url] = create_engine(  # ← SET
        self.database_url,
        echo=False,
        future=True,
        isolation_level="SERIALIZABLE",
        connect_args={"check_same_thread": False},
    )
self._engine = MEMORY_ENGINES[self.database_url]

This is not atomic. Between the not in check and the = assignment, another thread can execute the same block and also create an engine. The last writer wins — but the first thread already captured a reference to a different engine that is now orphaned.

Issue B — Inconsistent Engine Configurations

unit_of_work.py creates engines with:

isolation_level="SERIALIZABLE" ✅
future=True (SQLAlchemy 2.0 style) ✅
echo=False

migration_runner.py creates engines with:

No isolation_level ❌ (defaults to database-level, not SERIALIZABLE)
No future flag ❌
No echo

# migration_runner.py lines 248-253
if self.database_url not in MEMORY_ENGINES:
    MEMORY_ENGINES[self.database_url] = create_engine(  # DIFFERENT settings!
        self.database_url,
        connect_args={"check_same_thread": False},
    )
engine = MEMORY_ENGINES[self.database_url]

Whichever caller runs first will determine the engine settings. If MigrationRunner.init_or_upgrade() runs before UnitOfWork.engine, the shared engine will lack isolation_level="SERIALIZABLE" and future=True, despite all unit-of-work code expecting serializable isolation for proper rollback behavior.

Evidence

engine_cache.py (the module with the shared mutable state):

# src/cleveragents/infrastructure/database/engine_cache.py, line 15
MEMORY_ENGINES: dict[str, Engine] = {}  # No lock, no thread safety

unit_of_work.py (lines 76-84):

if self.database_url not in MEMORY_ENGINES:
    MEMORY_ENGINES[self.database_url] = create_engine(
        self.database_url,
        echo=False,
        future=True,
        isolation_level="SERIALIZABLE",      # ← SERIALIZABLE
        connect_args={"check_same_thread": False},
    )
self._engine = MEMORY_ENGINES[self.database_url]

migration_runner.py (lines 248-253):

if self.database_url not in MEMORY_ENGINES:
    MEMORY_ENGINES[self.database_url] = create_engine(
        self.database_url,
        connect_args={"check_same_thread": False},
        # ← NO isolation_level, NO future=True
    )
engine = MEMORY_ENGINES[self.database_url]

Expected Behavior

MEMORY_ENGINES should be thread-safe — engine creation should be atomic (e.g. using a threading.Lock).
All callers should create engines with the same settings (or a single factory function should be responsible for engine creation).
The canonical settings should include isolation_level="SERIALIZABLE" and future=True as used by unit_of_work.py (which actually runs queries).

Actual Behavior

TOCTOU: two concurrent callers can each create an engine; one gets discarded, the other is orphaned.
Whichever of MigrationRunner or UnitOfWork populates the cache first determines the engine settings for all subsequent callers — including whether SERIALIZABLE isolation is in effect.

Suggested Fix

Introduce a module-level lock and a single engine-creation factory in engine_cache.py:

# engine_cache.py
import threading
from sqlalchemy import create_engine
from sqlalchemy.engine import Engine

MEMORY_ENGINES: dict[str, Engine] = {}
_MEMORY_ENGINES_LOCK = threading.Lock()

def get_or_create_memory_engine(url: str, **kwargs: Any) -> Engine:
    """Thread-safe get-or-create for in-memory SQLite engines."""
    with _MEMORY_ENGINES_LOCK:
        if url not in MEMORY_ENGINES:
            MEMORY_ENGINES[url] = create_engine(url, **kwargs)
        return MEMORY_ENGINES[url]

And consolidate all callers to use a single canonical configuration.

TDD Note

After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD. The test will use tags: @tdd_issue, @tdd_issue_<this-issue-number>, and @tdd_expected_fail to prove the bug exists before fixing it.

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

## Bug Report: Concurrency — `MEMORY_ENGINES` global cache has race condition and creates engines with different settings ### Severity Assessment - **Impact**: In multi-threaded usage (e.g. during test runs or async workers), two threads can simultaneously observe the `sqlite:///:memory:` key missing from `MEMORY_ENGINES` and **each create a separate in-memory SQLite engine**. The second thread's engine overwrites the first in the dict. The first thread now holds a reference to an engine that no other code knows about — schema migrations may run against a different engine than the one actually used. Additionally, the same key may be populated with engines having **different configurations** depending on which caller runs first. - **Likelihood**: Likely in parallel test execution (e.g. `pytest-xdist`) and possible in any multi-threaded process that uses in-memory SQLite. Silent corruption with no error raised. - **Priority**: Medium ### Location - **File 1**: `src/cleveragents/infrastructure/database/engine_cache.py` - **Function/Class**: Module-level `MEMORY_ENGINES` dict - **Lines**: 15 (definition) - **File 2**: `src/cleveragents/infrastructure/database/unit_of_work.py` - **Function/Class**: `UnitOfWork.engine` property - **Lines**: 76–84 - **File 3**: `src/cleveragents/infrastructure/database/migration_runner.py` - **Function/Class**: `MigrationRunner.init_or_upgrade()` - **Lines**: 248–253 --- ### Description #### Issue A — TOCTOU Race Condition Both `unit_of_work.py` and `migration_runner.py` use a **check-then-set** pattern to populate `MEMORY_ENGINES`: ```python # unit_of_work.py lines 76-84 if self.database_url not in MEMORY_ENGINES: # ← CHECK MEMORY_ENGINES[self.database_url] = create_engine( # ← SET self.database_url, echo=False, future=True, isolation_level="SERIALIZABLE", connect_args={"check_same_thread": False}, ) self._engine = MEMORY_ENGINES[self.database_url] ``` This is not atomic. Between the `not in` check and the `=` assignment, another thread can execute the same block and also create an engine. The last writer wins — but the first thread already captured a reference to a **different** engine that is now orphaned. #### Issue B — Inconsistent Engine Configurations `unit_of_work.py` creates engines with: - `isolation_level="SERIALIZABLE"` ✅ - `future=True` (SQLAlchemy 2.0 style) ✅ - `echo=False` `migration_runner.py` creates engines with: - **No `isolation_level`** ❌ (defaults to database-level, not SERIALIZABLE) - **No `future` flag** ❌ - No `echo` ```python # migration_runner.py lines 248-253 if self.database_url not in MEMORY_ENGINES: MEMORY_ENGINES[self.database_url] = create_engine( # DIFFERENT settings! self.database_url, connect_args={"check_same_thread": False}, ) engine = MEMORY_ENGINES[self.database_url] ``` Whichever caller runs first will determine the engine settings. If `MigrationRunner.init_or_upgrade()` runs before `UnitOfWork.engine`, the shared engine will **lack `isolation_level="SERIALIZABLE"` and `future=True`**, despite all unit-of-work code expecting serializable isolation for proper rollback behavior. --- ### Evidence `engine_cache.py` (the module with the shared mutable state): ```python # src/cleveragents/infrastructure/database/engine_cache.py, line 15 MEMORY_ENGINES: dict[str, Engine] = {} # No lock, no thread safety ``` `unit_of_work.py` (lines 76-84): ```python if self.database_url not in MEMORY_ENGINES: MEMORY_ENGINES[self.database_url] = create_engine( self.database_url, echo=False, future=True, isolation_level="SERIALIZABLE", # ← SERIALIZABLE connect_args={"check_same_thread": False}, ) self._engine = MEMORY_ENGINES[self.database_url] ``` `migration_runner.py` (lines 248-253): ```python if self.database_url not in MEMORY_ENGINES: MEMORY_ENGINES[self.database_url] = create_engine( self.database_url, connect_args={"check_same_thread": False}, # ← NO isolation_level, NO future=True ) engine = MEMORY_ENGINES[self.database_url] ``` --- ### Expected Behavior 1. `MEMORY_ENGINES` should be thread-safe — engine creation should be atomic (e.g. using a `threading.Lock`). 2. All callers should create engines with the **same settings** (or a single factory function should be responsible for engine creation). 3. The canonical settings should include `isolation_level="SERIALIZABLE"` and `future=True` as used by `unit_of_work.py` (which actually runs queries). ### Actual Behavior 1. TOCTOU: two concurrent callers can each create an engine; one gets discarded, the other is orphaned. 2. Whichever of `MigrationRunner` or `UnitOfWork` populates the cache first determines the engine settings for all subsequent callers — including whether SERIALIZABLE isolation is in effect. --- ### Suggested Fix Introduce a module-level lock and a single engine-creation factory in `engine_cache.py`: ```python # engine_cache.py import threading from sqlalchemy import create_engine from sqlalchemy.engine import Engine MEMORY_ENGINES: dict[str, Engine] = {} _MEMORY_ENGINES_LOCK = threading.Lock() def get_or_create_memory_engine(url: str, **kwargs: Any) -> Engine: """Thread-safe get-or-create for in-memory SQLite engines.""" with _MEMORY_ENGINES_LOCK: if url not in MEMORY_ENGINES: MEMORY_ENGINES[url] = create_engine(url, **kwargs) return MEMORY_ENGINES[url] ``` And consolidate all callers to use a single canonical configuration. --- ### Category `concurrency` / `resource-management` --- ### TDD Note After this bug issue is verified, a corresponding Type/Testing issue will be created for TDD. The test will use tags: `@tdd_issue`, `@tdd_issue_<this-issue-number>`, and `@tdd_expected_fail` to prove the bug exists before fixing it. --- **Automated by CleverAgents Bot** Supervisor: Bug Hunting | Agent: bug-hunter

HAL9000 added the

labels

2026-04-09 21:08:15 +00:00

HAL9000 commented

2026-04-17 08:40:10 +00:00

Author

Owner

✅ Verified — Valid concurrency bug. TOCTOU race in global dict with inconsistent engine settings across callers. MoSCoW: Should Have — concurrency safety in shared state.

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

✅ **Verified** — Valid concurrency bug. TOCTOU race in global dict with inconsistent engine settings across callers. **MoSCoW: Should Have** — concurrency safety in shared state. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner-pool-supervisor

HAL9000 added this to the v3.5.0 milestone

2026-04-17 08:40:28 +00:00

HAL9000 added

and removed

labels

2026-04-18 09:11:30 +00:00