fix(database/migration_runner): add check_same_thread=False to get_current_revision() SQLite engine #10897

Merged
HAL9000 merged 2 commits from bugfix/m3-sqlite-check-same-thread into master 2026-05-05 17:48:41 +00:00
3 changed files with 122 additions and 0 deletions
@@ -0,0 +1,85 @@
"""Steps for TDD Issue #10507 — get_current_revision() SQLite threading fix."""
from __future__ import annotations
from typing import Any
from unittest.mock import MagicMock, patch
from behave import then, when
Outdated
Review

Suggestion: There is an extra blank line here (line 9) between the last import (from behave import then, when) and the first @when decorator. Per PEP 8 / ruff convention, module-level code should have exactly 2 blank lines between the top-level docstring block and imports, then exactly 1 blank line between imports and the first definition. This is likely what is causing the CI lint failure.

Suggestion: There is an extra blank line here (line 9) between the last import (`from behave import then, when`) and the first `@when` decorator. Per PEP 8 / ruff convention, module-level code should have exactly 2 blank lines between the top-level docstring block and imports, then exactly 1 blank line between imports and the first definition. This is likely what is causing the CI lint failure.
def _run_get_current_revision_and_capture_kwargs(
context: Any,
) -> None:
"""Shared helper: call get_current_revision() and capture create_engine kwargs.
Mocks ``create_engine`` and ``MigrationContext.configure`` so the call
completes without a real database. The keyword arguments passed to
``create_engine`` are stored on ``context.engine_creation_kwargs`` for
subsequent assertion steps.
"""
fake_engine = MagicMock()
fake_connection = MagicMock()
fake_connection.__enter__ = MagicMock(return_value=fake_connection)
fake_connection.__exit__ = MagicMock(return_value=False)
fake_engine.connect.return_value = fake_connection
migration_ctx = MagicMock()
migration_ctx.get_current_revision.return_value = None
captured_kwargs: list[dict[str, Any]] = []
def fake_create_engine(url: str, **kwargs: Any) -> MagicMock:
captured_kwargs.append(kwargs)
return fake_engine
with (
patch(
"cleveragents.infrastructure.database.migration_runner.create_engine",
side_effect=fake_create_engine,
),
patch(
"cleveragents.infrastructure.database.migration_runner.MigrationContext.configure",
return_value=migration_ctx,
),
):
context.revision_result = context.runner.get_current_revision()
context.engine_creation_kwargs = captured_kwargs
@when("I request the current revision and capture the engine creation args")
def step_when_capture_engine_args(context: Any) -> None:
"""Call get_current_revision and capture the create_engine call arguments."""
_run_get_current_revision_and_capture_kwargs(context)
@then("the SQLite engine should be created with check_same_thread set to False")
def step_then_sqlite_engine_has_check_same_thread(context: Any) -> None:
"""Verify the SQLite engine was created with check_same_thread=False."""
assert len(context.engine_creation_kwargs) == 1, (
f"Expected exactly 1 create_engine call, got {len(context.engine_creation_kwargs)}"
)
kwargs = context.engine_creation_kwargs[0]
assert "connect_args" in kwargs, (
"Expected connect_args in create_engine kwargs for SQLite, "
f"but got kwargs: {kwargs}"
)
assert kwargs["connect_args"].get("check_same_thread") is False, (
"Expected check_same_thread=False in connect_args, "
Outdated
Review

Question: step_when_capture_engine_args and step_when_verify_thread_safe_args perform the same mock setup and capture pattern — identical fake_engine, fake_connection, migration_ctx, and fake_create_engine definitions. They only differ by the variable name for captured kwargs. Consider consolidating into a single step and parameterizing the assertion that follows, or removing one if it is redundant. This would reduce maintenance burden and the ~70 lines of duplication.

Question: `step_when_capture_engine_args` and `step_when_verify_thread_safe_args` perform the same mock setup and capture pattern — identical `fake_engine`, `fake_connection`, `migration_ctx`, and `fake_create_engine` definitions. They only differ by the variable name for captured kwargs. Consider consolidating into a single step and parameterizing the assertion that follows, or removing one if it is redundant. This would reduce maintenance burden and the ~70 lines of duplication.
f"but got: {kwargs['connect_args']}"
)
@then("the non-SQLite engine should be created without check_same_thread")
def step_then_non_sqlite_engine_no_check_same_thread(context: Any) -> None:
"""Verify non-SQLite engines are not given check_same_thread."""
assert len(context.engine_creation_kwargs) == 1, (
f"Expected exactly 1 create_engine call, got {len(context.engine_creation_kwargs)}"
)
kwargs = context.engine_creation_kwargs[0]
connect_args = kwargs.get("connect_args", {})
assert "check_same_thread" not in connect_args, (
"Expected check_same_thread to be absent for non-SQLite engine, "
f"but got connect_args: {connect_args}"
)
@@ -0,0 +1,31 @@
@tdd_issue @tdd_issue_10507
Outdated
Review

Suggestion: The Background section (lines 1-19) reads like a technical design document rather than a Gherkin scenario description. While rich context is helpful, consider keeping the Background concise (what the test does) and moving the detailed technical explanation (threading root cause, check_same_thread mechanism) to a PR comment or issue note. If keeping it in the feature file, ensure it does not become stale — it mirrors the PR body closely and would need updating with every code change.

Suggestion: The Background section (lines 1-19) reads like a technical design document rather than a Gherkin scenario description. While rich context is helpful, consider keeping the Background concise (what the test *does*) and moving the detailed technical explanation (threading root cause, `check_same_thread` mechanism) to a PR comment or issue note. If keeping it in the feature file, ensure it does not become stale — it mirrors the PR body closely and would need updating with every code change.
Feature: TDD Issue #10507 — get_current_revision() must pass check_same_thread=False for SQLite
As a developer using MigrationRunner in a multi-threaded application
I want get_current_revision() to work safely from background threads
So that async startup flows and background migration checks do not crash
The root cause is that MigrationRunner.get_current_revision() calls
create_engine(self.database_url) without connect_args={"check_same_thread": False}
for SQLite databases. When called from a thread other than the one that
created the engine, SQLite raises:
ProgrammingError: SQLite objects created in a thread can only be
used in that same thread.
The sibling method init_or_upgrade() already passes check_same_thread=False
for SQLite, making this an inconsistency in the same class. Because
get_pending_migrations() and check_migrations_needed() both delegate to
get_current_revision(), the threading bug propagates to all three methods.
The fix adds connect_args={"check_same_thread": False} to the create_engine()
call in get_current_revision() when the database URL starts with "sqlite",
consistent with the existing pattern in init_or_upgrade().
Scenario: get_current_revision passes check_same_thread=False for SQLite engine
Given a migration runner configured for "sqlite:///:memory:"
When I request the current revision and capture the engine creation args
Then the SQLite engine should be created with check_same_thread set to False
Scenario: get_current_revision does not pass check_same_thread for non-SQLite engine
Given a migration runner configured for "postgresql://user:pass@localhost/testdb"
When I request the current revision and capture the engine creation args
Then the non-SQLite engine should be created without check_same_thread
@@ -151,6 +151,12 @@ class MigrationRunner:
def get_current_revision(self) -> str | None:
"""Get the current migration revision of the database.
For SQLite databases, the engine is created with
Outdated
Review

Question: init_or_upgrade() uses startswith("sqlite") for its check, but SQLite URLs can also be sqlite:/// (file-based, 3 slashes) or sqlite:///, sqlite:/// (absolute path with 4 slashes). The startswith("sqlite") correctly catches all of these since the schema prefix is always sqlite. Just confirming this is intentional — it will also match sqlite+custom:// dialects, which is the intended behavior.

Question: `init_or_upgrade()` uses `startswith("sqlite")` for its check, but SQLite URLs can also be `sqlite:///` (file-based, 3 slashes) or `sqlite:///`, `sqlite:///` (absolute path with 4 slashes). The `startswith("sqlite")` correctly catches all of these since the schema prefix is always `sqlite`. Just confirming this is intentional — it will also match `sqlite+custom://` dialects, which is the intended behavior.
``connect_args={"check_same_thread": False}`` so that this method
can be safely called from any thread — including background threads
used in async startup flows. This is consistent with the pattern
used in :meth:`init_or_upgrade`.
Returns:
Current revision ID or None if no migrations have been applied
"""