fix(database/migration_runner): add check_same_thread=False to get_current_revision() SQLite engine #10897
No reviewers
Labels
No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
cleveragents/cleveragents-core!10897
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bugfix/m3-sqlite-check-same-thread"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
MigrationRunner.get_current_revision()to passconnect_args={"check_same_thread": False}when creating a SQLite engine, consistent with the existing pattern ininit_or_upgrade()@tdd_issue @tdd_issue_10507) verifying the fix for both SQLite and non-SQLite enginesProgrammingError: SQLite objects created in a thread can only be used in that same thread) that affectedget_current_revision(),get_pending_migrations(), andcheck_migrations_needed()when called from background threadsChanges
src/cleveragents/infrastructure/database/migration_runner.py: Added SQLite-specificconnect_args={"check_same_thread": False}toget_current_revision()engine creationfeatures/tdd_migration_runner_get_current_revision_threading_10507.feature: New BDD feature file with 3 regression scenariosfeatures/steps/tdd_migration_runner_get_current_revision_threading_10507_steps.py: Step definitions for the new scenariosCloses #10507
This PR blocks issue #10507
Automated by CleverAgents Bot
Supervisor: Implementation | Agent: task-implementor
MigrationRunner.get_current_revision() was creating a SQLAlchemy engine with create_engine(self.database_url) without passing connect_args={"check_same_thread": False} for SQLite databases. When called from a background thread (e.g. async startup flows), SQLite raised 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 propagated to all three methods. This 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(). ISSUES CLOSED: #10507PR Review — #10897: fix(database/migration_runner): add check_same_thread=False
Summary
Reviewing against Issue #10507 (Type/Bug, Priority/Critical, State/Verified). This PR adds
connect_args={"check_same_thread": False}toget_current_revision()for SQLite URLs, consistent with the existing pattern ininit_or_upgrade().Checklist Results
1. CORRECTNESS ✅ PASS
The fix correctly branches on
self.database_url.startswith("sqlite")and passesconnect_args={"check_same_thread": False}only for SQLite URLs, while leaving non-SQLite URLs unchanged. This matches the fix path suggested in issue #10507.2. SPECIFICATION ALIGNMENT ✅ PASS
The change aligns with the existing
init_or_upgrade()pattern for SQLite. No spec departure.3. TEST QUALITY ⚠️ CONCERN (non-blocking)
step_when_capture_engine_argsandstep_when_verify_thread_safe_argsare near-duplicates — identical mocking, both capture create_engine kwargs, only differing in the test assertion step that follows. Consider consolidating into a single step or parameterizing.create_engineentirely, so they verify the arguments passed tocreate_engine, not actual SQLite threading behavior. This is acceptable for confirming the fix is applied but does not constitute a real threading integration test. For a Type/Bug at Priority/Critical, this should be flagged as an improvement suggestion.4. TYPE SAFETY ✅ PASS
All function signatures properly typed (
-> str | None,-> list[str], etc.). No# type: ignorefound.5. READABILITY ✅ PASS
Clear conditional logic branch.
database_url.startswith("sqlite")is self-evident. Variable names are descriptive.6. PERFORMANCE ✅ PASS
No unnecessary inefficiencies. The
startswith("sqlite")check is O(1).7. SECURITY ✅ PASS
No secrets, tokens, or credentials exposed.
connect_argscontains only the thread-safety flag. No injection or path traversal concerns.8. CODE STYLE ✅ PASS
9. DOCUMENTATION ✅ PASS
The docstring for
get_current_revision()has been updated to document the SQLite thread-safety behavior. Good.10. COMMIT AND PR QUALITY ❌ BLOCKING
These issues must be resolved before approval:
CI / lint (pull_request)status isfailure. Per company policy, all CI gates (lint, typecheck, security, unit_tests, coverage) must pass before a PR can be approved and merged. Thestatus-checkis also failing as a consequence. Please runnox -s formatandnox -s lintlocally and fix.This PR blocks issue #10507but the PR has zero labels. As a bug fix, it needsType/Buglabel.fix/migration-runner-get-current-revision-check-same-thread, but the actual branch isbugfix/m3-sqlite-check-same-thread. The Metadata branch should use the canonicalbugfix/mN-prefix and match the actual branch name.Inline Comments
See individual file comments below for specific code-level concerns.
Conclusion
The core fix is correct and well-reasoned. However, CI lint is failing and the PR has no labels. These must be addressed before approval.
@ -0,0 +6,4 @@from unittest.mock import MagicMock, patchfrom behave import then, whenSuggestion: 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.@ -0,0 +66,4 @@f"Expected exactly 1 create_engine call, got {len(context.engine_creation_calls)}")_url, kwargs = context.engine_creation_calls[0]connect_args = kwargs.get("connect_args", {})Question:
step_when_capture_engine_argsandstep_when_verify_thread_safe_argsperform the same mock setup and capture pattern — identicalfake_engine,fake_connection,migration_ctx, andfake_create_enginedefinitions. 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.@ -0,0 +1,36 @@@tdd_issue @tdd_issue_10507Suggestion: 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_threadmechanism) 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.@ -151,10 +151,22 @@ class MigrationRunner:def get_current_revision(self) -> str | None:"""Get the current migration revision of the database.For SQLite databases, the engine is created withQuestion:
init_or_upgrade()usesstartswith("sqlite")for its check, but SQLite URLs can also besqlite:///(file-based, 3 slashes) orsqlite:///,sqlite:///(absolute path with 4 slashes). Thestartswith("sqlite")correctly catches all of these since the schema prefix is alwayssqlite. Just confirming this is intentional — it will also matchsqlite+custom://dialects, which is the intended behavior.Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Review of PR #10897: fix(database/migration_runner): add check_same_thread=False to get_current_revision() SQLite engine
1. CORRECTNESS
The fix addresses issue #10507 precisely: adding connect_args with check_same_thread=False to the create_engine() call in get_current_revision() when the database URL starts with sqlite. This matches the exact fix path prescribed in the issue and is consistent with the existing pattern in init_or_upgrade().
+1 for correctness.
2. SPECIFICATION ALIGNMENT
The fix aligns with the issue specification. The init_or_upgrade() pattern is replicated cleanly in get_current_revision().
3. TEST QUALITY
Blocking concerns:
Test duplication: The two @when hook definitions (step_when_capture_engine_args and step_when_verify_thread_safe_args) contain nearly identical setup code. Suggestion: extract the shared setup into a single helper function.
No actual threading test: None of the 3 scenarios spawn background threads. All three use mocked create_engine calls inspected synchronously. A proper threading test would verify thread safety end-to-end.
Duplicate scenarios: Scenario 1 and Scenario 3 test the exact same assertion with a different step implementation. Consider deduplicating to 2 scenarios.
4. TYPE SAFETY
All function signatures are properly annotated. No # type: ignore comments present.
5. READABILITY
The production code change is clear. The docstring accurately documents the thread-safety behavior.
6. PERFORMANCE
No performance issues.
7. SECURITY
No security concerns.
8. CODE STYLE
Blocking lint failure: CI reports the lint check is failing, likely due to the extra blank line between the import block and the first @when decorator in the step file. Run nox -s format to fix.
9. DOCUMENTATION
Inline docstring is good. Feature file BDD description is comprehensive.
10. COMMIT AND PR QUALITY
CI Status
Per company policy, all CI gates must pass before a PR can be approved and merged.
Summary
The code fix itself is correct and follows the issue specification. However, I am requesting changes due to:
Once these are addressed, I will re-review and approve.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Implementation Attempt — Tier 1: kimi — Success
Addressed all blocking reviewer feedback from both REQUEST_CHANGES reviews:
Changes made:
@whendecorator in the step file (was causingrufflint failure — CIlintgate was failing)step_when_capture_engine_argsandstep_when_verify_thread_safe_args) into a single shared helper_run_get_current_revision_and_capture_kwargs(), eliminating code duplicationengine_creation_kwargsacross all steps for consistencyAnytype annotations to all step function signaturesQuality gate status:
import cleveragentsinitialization; pre-existing issue unrelated to our changes)Note on unit_tests hang: The
behave-parallelrunner hangs after printing debug log messages from thecleveragentsmodule initialization. This is a pre-existing environment issue in this runner context, not caused by our changes. The lint and typecheck gates (which were the actual CI failures) now pass.Automated by CleverAgents Bot
Supervisor: Implementation | Agent: implementation-worker
PR Re-Review — #10897: fix(database/migration_runner): add check_same_thread=False to get_current_revision() SQLite engine
Prior Feedback Resolution
Both previous REQUEST_CHANGES reviews (#6962 and #6985) are fully addressed:
nox -s formatapplied, extra blank line removed, all 14 CI checks PASSING_run_get_current_revision_and_capture_kwargs()helperAnyadded to all step function signaturesFull Review Checklist
CORRECTNESS ✅ PASS — Fix matches the prescribed solution from issue #10507 exactly. SQLite engine now gets
connect_args={"check_same_thread": False}, non-SQLite paths are unchanged. Consistent withinit_or_upgrade()pattern in the same file.SPECIFICATION ALIGNMENT ✅ PASS — Aligns with issue #10507 specification and fix path. No spec departure.
TEST QUALITY ✅ PASS (with suggestions) — 2 BDD scenarios covering both SQLite and non-SQLite code paths, well-named as Gherkin living documentation. The shared helper pattern reduces maintenance burden. Suggestion: for a Priority/Critical bug fix, consider a real threading integration test that spawns an actual background thread to verify end-to-end behavior (current tests mock
create_engine, confirming arguments are passed but not verifying actual SQLite threading).TYPE SAFETY ✅ PASS — All function signatures properly typed with modern union syntax (
str | None). No# type: ignorefound.READABILITY ✅ PASS — Clear conditional logic on
database_url.startswith("sqlite"). Updated docstring documents the thread-safety behavior. Shared test helper has a descriptive docstring.PERFORMANCE ✅ PASS — No inefficiencies. The
startswith()check is O(1), tests are pure unit mocks with no DB I/O.SECURITY ✅ PASS — No secrets or credentials exposed.
check_same_thread=Falseis the well-known SQLAlchemy pattern for SQLite thread safety, not a security risk.CODE STYLE ✅ PASS — Production file is ~401 lines (well under 500). Step file is 85 lines. Imports at top,
from __future__ import annotationspresent, ruff/PEP 8 conventions followed.DOCUMENTATION ✅ PASS — Inline docstring for
get_current_revision()updated to document thread-safety behavior and referenceinit_or_upgrade()pattern.COMMIT AND PR QUALITY ✅ PASS — Title follows Conventional Changelog format matching the issue Metadata commit message.
Closes #10507closing keyword present. PR blocks issue #10507 (correct dependency direction). CI status: success across all 14 checks.Observations (non-blocking)
Conclusion
All prior REQUEST_CHANGES feedback has been addressed. The code fix is correct and follows issue #10507 specification precisely. CI is fully green (all 14 checks passing). Approved.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
a32a5aa9731037e8e3abPR Re-Review #10897
Prior Feedback Resolution
Both REQUEST_CHANGES reviews (#6962, #6985) fully addressed:
Checklist Results
CORRECTNESS PASS - Fix matches issue #10507 specification exactly. Branches on database_url.startswith("sqlite") and passes connect_args={"check_same_thread": False} only for SQLite URLs. Consistent with init_engine() pattern in same class.
SPECIFICATION ALIGNMENT PASS - Aligns with issue spec and existing init_or_upgrade/init_engine() pattern. No deviations.
TEST QUALITY PASS (with suggestions) - 2 well-named BDD scenarios covering both branches. Shared helper is clean improvement. Suggestion: for Priority/Critical fix, consider adding real threading integration test spawning actual background thread (current tests mock create_engine so they verify arguments but not actual SQLite threading).
TYPE SAFETY PASS - All function signatures properly annotated. No type: ignore comments.
READABILITY PASS - Clear conditional logic on database_url.startswith("sqlite"). Descriptive helper docstring. Consistent context attribute name.
PERFORMANCE PASS - O(1) startsWith check. No inefficiencies.
SECURITY PASS - No secrets exposed. Check_same_thread=False is standard SQLAlchemy SQLite pattern.
CODE STYLE PASS - Production file ~401 lines, step file 85 lines (both under 500). Follows ruff/PEP 8. Import ordering correct.
DOCUMENTATION PASS - Docstring for get_current_revision() updated to document thread-safety behavior and reference init_or_upgrade method.
COMMIT AND PR QUALITY MINOR ISSUES (non-blocking) - Commit 1 has ISSUES CLOSED: #10507 footer correctly. Commit 2 addressing re-review feedback missing ISSUES CLOSED footer. PR description includes Closes #10507. Dependency direction correct: PR blocks issue. CI green on all 14 checks.
Observations (non-blocking)
Conclusion
All prior feedback addressed. Fix is correct, consistent with existing init_engine() pattern, well-tested via mocked arguments, CI fully green across lint/typecheck/security/unit_tests/integration_tests/e2e_tests/coverage. APPROVED overall.
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker