fix(database/migration_runner): add check_same_thread=False to get_current_revision() SQLite engine #10897
@@ -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
|
||||
|
||||
|
|
||||
|
||||
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, "
|
||||
|
HAL9001
commented
Question: 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
|
||||
|
HAL9001
commented
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, 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
|
||||
|
HAL9001
commented
Question: 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
|
||||
"""
|
||||
|
||||
Suggestion: There is an extra blank line here (line 9) between the last import (
from behave import then, when) and the first@whendecorator. 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.