BUG-HUNT: [concurrency] Shared Mutable State in SqliteChangeSetStore #3025

New issue

Open

opened 2026-04-05 04:05:21 +00:00 by freemo · 1 comment

freemo commented

2026-04-05 04:05:21 +00:00

Owner

Metadata

Branch: bugfix/m4-sqlite-changeset-store-thread-safety
Commit Message: fix(concurrency): protect _plan_map with threading.Lock in SqliteChangeSetStore
Milestone: v3.3.0
Parent Epic: #362

Bug Report: [concurrency] — Shared Mutable State in SqliteChangeSetStore

Severity Assessment

Impact: Race conditions leading to inconsistent data or unexpected errors in a multi-threaded environment.
Likelihood: Medium. This depends on whether SqliteChangeSetStore is used as a singleton in a multi-threaded context.
Priority: Medium

Location

File: src/cleveragents/infrastructure/database/changeset_repository.py
Function/Class: SqliteChangeSetStore
Lines: 407

Description

The _plan_map dictionary in SqliteChangeSetStore is a mutable attribute that is accessed by the start and get methods without any synchronization mechanism. If a single instance of SqliteChangeSetStore is shared across multiple threads, this can lead to race conditions.

Evidence

class SqliteChangeSetStore:
    """SQLite-backed ``ChangeSetStore`` implementation.
    ...
    """

    def __init__(
        self,
        session_factory: Callable[[], Session],
    ) -> None:
        if session_factory is None:
            raise ValueError("session_factory must not be None")
        self._entry_repo = ChangeSetEntryRepository(session_factory)
        self._plan_map: dict[str, str] = {}

    def start(self, plan_id: str) -> str:
        """Create a new empty ChangeSet for *plan_id*."""
        if not plan_id:
            raise ValueError("plan_id must not be empty")
        changeset_id = str(ULID())
        self._plan_map[changeset_id] = plan_id
        return changeset_id
...
    def get(self, changeset_id: str) -> SpecChangeSet | None:
        ...
        if not entries:
            plan_id = self._plan_map.get(changeset_id, "")
            ...

Expected Behavior

Access to shared mutable state should be protected by a lock to ensure thread safety.

Actual Behavior

The _plan_map dictionary is accessed without a lock, creating a potential race condition.

Suggested Fix

Introduce a threading.Lock to protect access to _plan_map.

import threading

class SqliteChangeSetStore:
    def __init__(
        self,
        session_factory: Callable[[], Session],
    ) -> None:
        if session_factory is None:
            raise ValueError("session_factory must not be None")
        self._entry_repo = ChangeSetEntryRepository(session_factory)
        self._plan_map: dict[str, str] = {}
        self._lock = threading.Lock()

    def start(self, plan_id: str) -> str:
        if not plan_id:
            raise ValueError("plan_id must not be empty")
        changeset_id = str(ULID())
        with self._lock:
            self._plan_map[changeset_id] = plan_id
        return changeset_id

    def get(self, changeset_id: str) -> SpecChangeSet | None:
        if not changeset_id:
            return None

        entries = self._entry_repo.get_entries_for_changeset(
            changeset_id,
        )
        if not entries:
            with self._lock:
                plan_id = self._plan_map.get(changeset_id, "")
            if not plan_id:
                return None
            return SpecChangeSet(
                changeset_id=changeset_id,
                plan_id=plan_id,
            )
...

Subtasks

Add import threading to changeset_repository.py
Add self._lock = threading.Lock() in SqliteChangeSetStore.__init__
Wrap self._plan_map[changeset_id] = plan_id in start() with with self._lock:
Wrap self._plan_map.get(changeset_id, "") in get() with with self._lock:
Write Behave unit tests covering concurrent access to start() and get() from multiple threads
Verify all nox stages pass

Definition of Done

threading.Lock introduced and all _plan_map accesses are protected
Behave unit tests added covering the thread-safety fix (concurrent start/get calls)
No regressions in existing test suite
All nox stages pass
Coverage >= 97%

Automated by CleverAgents Bot
Supervisor: Bug Hunting | Agent: ca-new-issue-creator

## Metadata - **Branch**: `bugfix/m4-sqlite-changeset-store-thread-safety` - **Commit Message**: `fix(concurrency): protect _plan_map with threading.Lock in SqliteChangeSetStore` - **Milestone**: v3.3.0 - **Parent Epic**: #362 ## Bug Report: [concurrency] — Shared Mutable State in SqliteChangeSetStore ### Severity Assessment - **Impact**: Race conditions leading to inconsistent data or unexpected errors in a multi-threaded environment. - **Likelihood**: Medium. This depends on whether `SqliteChangeSetStore` is used as a singleton in a multi-threaded context. - **Priority**: Medium ### Location - **File**: `src/cleveragents/infrastructure/database/changeset_repository.py` - **Function/Class**: `SqliteChangeSetStore` - **Lines**: 407 ### Description The `_plan_map` dictionary in `SqliteChangeSetStore` is a mutable attribute that is accessed by the `start` and `get` methods without any synchronization mechanism. If a single instance of `SqliteChangeSetStore` is shared across multiple threads, this can lead to race conditions. ### Evidence ```python class SqliteChangeSetStore: """SQLite-backed ``ChangeSetStore`` implementation. ... """ def __init__( self, session_factory: Callable[[], Session], ) -> None: if session_factory is None: raise ValueError("session_factory must not be None") self._entry_repo = ChangeSetEntryRepository(session_factory) self._plan_map: dict[str, str] = {} def start(self, plan_id: str) -> str: """Create a new empty ChangeSet for *plan_id*.""" if not plan_id: raise ValueError("plan_id must not be empty") changeset_id = str(ULID()) self._plan_map[changeset_id] = plan_id return changeset_id ... def get(self, changeset_id: str) -> SpecChangeSet | None: ... if not entries: plan_id = self._plan_map.get(changeset_id, "") ... ``` ### Expected Behavior Access to shared mutable state should be protected by a lock to ensure thread safety. ### Actual Behavior The `_plan_map` dictionary is accessed without a lock, creating a potential race condition. ### Suggested Fix Introduce a `threading.Lock` to protect access to `_plan_map`. ```python import threading class SqliteChangeSetStore: def __init__( self, session_factory: Callable[[], Session], ) -> None: if session_factory is None: raise ValueError("session_factory must not be None") self._entry_repo = ChangeSetEntryRepository(session_factory) self._plan_map: dict[str, str] = {} self._lock = threading.Lock() def start(self, plan_id: str) -> str: if not plan_id: raise ValueError("plan_id must not be empty") changeset_id = str(ULID()) with self._lock: self._plan_map[changeset_id] = plan_id return changeset_id def get(self, changeset_id: str) -> SpecChangeSet | None: if not changeset_id: return None entries = self._entry_repo.get_entries_for_changeset( changeset_id, ) if not entries: with self._lock: plan_id = self._plan_map.get(changeset_id, "") if not plan_id: return None return SpecChangeSet( changeset_id=changeset_id, plan_id=plan_id, ) ... ``` ### Category concurrency ## Subtasks - [ ] Add `import threading` to `changeset_repository.py` - [ ] Add `self._lock = threading.Lock()` in `SqliteChangeSetStore.__init__` - [ ] Wrap `self._plan_map[changeset_id] = plan_id` in `start()` with `with self._lock:` - [ ] Wrap `self._plan_map.get(changeset_id, "")` in `get()` with `with self._lock:` - [ ] Write Behave unit tests covering concurrent access to `start()` and `get()` from multiple threads - [ ] Verify all nox stages pass ## Definition of Done - [ ] `threading.Lock` introduced and all `_plan_map` accesses are protected - [ ] Behave unit tests added covering the thread-safety fix (concurrent `start`/`get` calls) - [ ] No regressions in existing test suite - [ ] All nox stages pass - [ ] Coverage >= 97% --- **Automated by CleverAgents Bot** Supervisor: Bug Hunting | Agent: ca-new-issue-creator

freemo added this to the v3.3.0 milestone

2026-04-05 04:06:35 +00:00

freemo added a new dependency

2026-04-05 04:07:13 +00:00

#362 Epic: Security & Safety Hardening

freemo commented

2026-04-05 04:08:50 +00:00

Author

Owner

Issue triaged by project owner:

State: Verified
Priority: Confirmed
MoSCoW: Should Have

Valid finding verified during batch triage.

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

Issue triaged by project owner: - **State**: Verified - **Priority**: Confirmed - **MoSCoW**: Should Have Valid finding verified during batch triage. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner

freemo referenced this issue

2026-04-05 06:15:22 +00:00

[CA-AUTO] UAT Pool Supervisor — v3.2.0–v3.7.0 — Session Tracker #3094